| Summary: | Signed overflow in openbsd-compat/strlcpy.c | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | Portable OpenSSH | Reporter: | Yuanjie Huang <yuanjie.huang> | ||||||||
| Component: | Miscellaneous | Assignee: | Assigned to nobody <unassigned-bugs> | ||||||||
| Status: | CLOSED WONTFIX | ||||||||||
| Severity: | enhancement | CC: | djm, dtucker, hongxu.jia | ||||||||
| Priority: | P5 | ||||||||||
| Version: | 7.3p1 | ||||||||||
| Hardware: | All | ||||||||||
| OS: | Linux | ||||||||||
| Attachments: |
|
||||||||||
Created attachment 2867 [details]
proposed fix.
Thanks. Looking (these files are sourced from OpenBSD, so any changes will need to go upstream first). I applied then, after some discussion with some folks, reverted your change. The rationale is that C11 6.5.6.9 says: """ When two pointers are subtracted, both shall point to elements of the same array object, or one past the last element of the array object; the result is the difference of the subscripts of the two array elements. """ In these cases the objects are arrays of char so the result is defined, and we believe that the compiler incorrectly trapping on defined behaviour. I also found https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63303 ("Pointer subtraction is broken when using -fsanitize=undefined") which seems to support this position. Created attachment 3293 [details]
proposed fix v2
I agree with Darren's analysis - the existing implementation is compliant with ISO C. closing resolved bugs as of 8.6p1 release |
Created attachment 2866 [details] test driver of strlcpy to demo the bug. Pointer arithmatic results in implementation defined signed integer type, so that 's - src' in strlcpy and others may trigger signed overflow. When the code is compiled by gcc or clang with -ftrapv option, the overflow would lead to program abort. $ gcc test.c strlcpy.c -o test -m32 -ftrapv $ ./test Allocating src starting from 0x7fffff40 Trying to copy 0x400 from 0x7fffff40 to 0x9630008 Aborted (core dumped) The proposed fix is also attached.