fix(patch): memory unsafe file read on Linux #234
Conversation
When reading from the file descriptor into the temporary buffer, it was only by happenstance that the buffer would contain a terminating nul byte. The buffer is uninitialized and thus may contain any data. And the `/proc` files that are read into the buffer are **not** nul terminated. If the temporary buffer happened to be zero-initialized, then this would accidentally work correctly. If the temporary buffer happend to have a nul byte somewhere after the read data and in-bounds, then the end of the resulting String would be corrupted with arbitrary crud. This crud would ostensibly be after a terminating newline and thus parsers might have ignored it? If the temporary buffer happened to not have any nul bytes after the read data, then other Bad Things could happen, including silent success or a crash due to segmentation fault or various checks in String being violated. The repair is to append a nul byte after the read data (ensuring that the nul byte is not placed after the end of the buffer). Signed-off-by: Peter Grayson <pete@jpgrayson.net>
|
Great catch, thank you very much! Added an extra precondition for now as well if more data than should be possible comes in. |
hassila
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #234 +/- ##
==========================================
+ Coverage 64.99% 65.03% +0.04%
==========================================
Files 33 33
Lines 3462 3466 +4
==========================================
+ Hits 2250 2254 +4
Misses 1212 1212
Continue to review full report in Codecov by Sentry.
|
|
The CI failures on Linux/5.7 are really weird, |
|
Thanks for the contribution! |
|
Thank you for taking a look at this PR so quickly, @hassila. Much appreciated! |
|
The added Also, as written the |
|
If count would equal max buffer there might be more data available we didn't get, so it was on purpose it was written thus - in practice we should never hit that, then something is quite unexpected. |
|
Ah, I better understand the intent now. Thanks for explaining. |
Description
When reading from the file descriptor into the temporary buffer, it was only by happenstance that the buffer would contain a terminating nul byte.
The buffer is uninitialized and thus may contain any data.
And the
/procfiles that are read into the buffer are not nul terminated.If the temporary buffer happened to be zero-initialized, then this would accidentally work correctly.
If the temporary buffer happened to have a nul byte somewhere after the read data and in-bounds, then the end of the resulting String would be corrupted with arbitrary crud. This crud would ostensibly be after a terminating newline and thus parsers might have ignored it?
If the temporary buffer happened to not have any nul bytes after the read data, then other Bad Things could happen, including silent success or a crash due to segmentation fault or various checks in String being violated.
The repair is to append a nul byte after the read data (ensuring that the nul byte is not placed after the end of the buffer).
What motivated me to go look for this issue is that I have a benchmark suite in a non-public project that crashes due to this issue. That benchmark suite runs lots of iterations and somehow seems to have a memory access pattern that regularly leads to the conditions where the ill-effects of this bug can manifest.
How Has This Been Tested?
Regarding testing, I did some one-off testing that pre-filled the temporary buffer with non-zero bytes and then observed those bytes were erroneously present in the resulting
String.I have not attempted to craft a unit test that triggers this bug. That would require either directly or indirectly manipulating the system allocator to affect the initial state of this temporary buffer. Too much effort, IMO.
Minimal checklist:
DocCcode-level documentation for any public interfaces exported by the package