overlapping buffers and memccpy()
#78
Comments
|
I instrumented the code to write a diagnostic message when Many others overlap somewhat: Note the I have no idea why that function is being called with overlapping buffers. But it obviously broken by relying on |
|
I did a more fine grained analysis. First, note that 97.2% of the calls to Also, when the source and destination buffers a 8000 ren't the same (i.e., have different starting addrs) it appears they never actually overlap. It only appears they might because a large length (typically 1024) is being passed in. But because the copy terminates when a null byte is seen, and that almost always happens within the first 100 bytes, there isn't any overlap when the starting addr of each buffer differs. Of the invocations that theoretically have overlapping buffers only 27.4% actually overlap. And those are suspicious given that the source and destination addrs are the same and thus should be a nop. Meaning it should be okay to elide the copy. |
|
Note that ksh cannot be built with the There are a significant number of problems with the code. Most notably with respect to functions which have more than one declaration which do not agree with each other. Something |
|
I just noticed issue #4 for which this issue is essentially a duplicate. My commit de00119 that was recently merged fixes both of the problems identified in issue #4 but in a slightly different manner than was proposed there. So issue #4 can be closed. However, as noted in my previous comments on this issue |
The behavior of the `memccpy()` function is undefined when the buffers overlap which is sometimes true for the `sfputr()` function. So always use an explicit loop to copy the data. Fixes #78
|
Closing since my change to remove the unsafe use of |
The behavior of the `memccpy()` function is undefined when the buffers overlap which is sometimes true for the `sfputr()` function. So always use an explicit loop to copy the data. Fixes #78
The sfputr() function (put out a null-terminated string) contained a use of memccpy() that was invalid and could cause crashes, because the buffer it was copying into could overlap or even be identical with the buffer being copied from. Among (probably) other things, this commit fixes a crash in 'print -v' (print a compound variable structure) on macOS, that caused the comvar.sh and comvario.sh regression tests to fail spectacularly. Now they pass. Issue discovered and fixed by Kurtis Rader in the abandoned ksh2020 branch; this commit backports the fix. He wrote: | #if _lib_memccpy && !__ia64 /* these guys may never get it right */ | | The problem is that assertion is wrong. It implies that the libc | implementation of memccpy() on IA64 is broken. Which is | incorrect. The problem is the AST sfputr() function is depending | on what is explicitly undefined behavior in the face of | overlapping source and destination buffers. | [...] Using memccpy() simply complicates the code and is unlikely | to be measurably, let alone noticeably, faster. Further discussion/analysis: att#78 src/lib/libast/sfio/sfputr.c: - Remove memccpy use. Always use the manual copying loop. (cherry picked from commit fbe3c83335256cb714a4aa21f555083c9f1d71d8)
The sfputr() function (put out a null-terminated string) contained a use of memccpy() that was invalid and could cause crashes, because the buffer it was copying into could overlap or even be identical with the buffer being copied from. Among (probably) other things, this commit fixes a crash in 'print -v' (print a compound variable structure) on macOS, that caused the comvar.sh and comvario.sh regression tests to fail spectacularly. Now they pass. Issue discovered and fixed by Kurtis Rader in the abandoned ksh2020 branch; this commit backports the fix. He wrote: | #if _lib_memccpy && !__ia64 /* these guys may never get it right */ | | The problem is that assertion is wrong. It implies that the libc | implementation of memccpy() on IA64 is broken. Which is | incorrect. The problem is the AST sfputr() function is depending | on what is explicitly undefined behavior in the face of | overlapping source and destination buffers. | [...] Using memccpy() simply complicates the code and is unlikely | to be measurably, let alone noticeably, faster. Further discussion/analysis: att#78 src/lib/libast/sfio/sfputr.c: - Remove memccpy use. Always use the manual copying loop. (cherry picked from commit fbe3c83335256cb714a4aa21f555083c9f1d71d8)
Some notes: - Removed a TODO note that was fixed in commit 43d9fba. - Removed a duplicate note about the '%l' time format in the changelog. - Applied the following documentation fixes from Terrence J. Doyle: - https://www.mail-archive.com/ast-developers@lists.research.att.com/msg01852.html - https://www.mail-archive.com/ast-developers@lists.research.att.com/msg01856.html - Fixed strange grammar in one of the error messages. - Added missing options for rksh to the synopsis section. - Applied a formatting fix from ksh93v- to the man page. - Replaced a C99 line comment in src/lib/libast/comp/realpath.c with a proper comment that is valid in C89. - Prioritize UTC over GMT in the documentation (missed by commit c9634e9). - Add some extra information for 'ksh -R file' to the man page. This patch is from Red Hat: https://git.centos.org/rpms/ksh/blob/c8/f/SOURCES/ksh-20080202-manfix.patch
The change I authored, commit de00119, which was recently merged to fix building on BSD based systems like macOS only works when using
gccto do the compilation. If you instead use theclangcompiler provided by Apple it fails because theBSDpreprocessor symbol isn't defined. You also need to blacklist__APPLE__.But here's the thing, the code before my change began with this preprocessor directive:
The problem is that assertion is wrong. It implies that the libc implementation of
memccpy()on IA64 is broken. Which is incorrect. The problem is the ASTsfputr()function is depending on what is explicitly undefined behavior in the face of overlapping source and destination buffers. Every implementation is free to optimize the operation by assuming the buffers do not overlap but in so doing produce incorrect results, including aborting the program, if the buffers do overlap.I was too timid with my previous change. I should not have added another blacklisted architecture. Instead the use of
memccpy()should simply be removed. If you look at the two alternatives usingmemccpy()simply complicates the code and is unlikely to be measurably, let alone noticeably, faster.The text was updated successfully, but these errors were encountered: