lpt: fix misprint fgets buffer size in cgroup parsing - #2059
Conversation
LinuxProcessTable_readCGroupFile() declares an inner buffer of PROC_LINE_LENGTH + 1 bytes but passes PROC_LINE_LENGTH to fgets(), telling it that the buffer is one byte smaller than it actually is
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
Suggested labels: Suggested reviewers: Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Wtf |
Download: 2220/3041 items 910.2/1252.2MB 72% complete (809k/s) |
ravi-arnan
left a comment
ravi-arnan
left a comment
There was a problem hiding this comment.
Correct, and safe either way: buffer is PROC_LINE_LENGTH + 1 bytes, so
sizeof(buffer) is within bounds and the old form simply left the last byte
unused. No overflow before or after.
Since I had a Linux box to hand I checked when the extra byte can actually be
observed, which turns out to be narrower than it looks.
It only matters for a /proc/<pid>/cgroup line of 4096 bytes or more. Those are
constructible: a delegated user cgroup plus 21 nested 200-character components
gives a 4099-byte line. It has to be built with relative chdir because the
absolute path is past PATH_MAX, but the kernel prints the full line regardless:
$ python3 -c "..." # chdir down 21 levels under user@1000.service, write pid
line bytes: 4099
On that process, both main and this branch abort before the byte matters:
main exit=134 FATAL PROGRAM ERROR / signal 6
pr2059 exit=134 same
That is #1913, the CGROUP column width crash that #2051 fixes, not anything
introduced here. Rebuilding this branch with #2051's buffer fix on top, and
rebuilding the same tree with only that fix and the fgets line reverted, both
run clean and render the same 255 characters, since the column truncates there
anyway:
with fgets(buffer, PROC_LINE_LENGTH) longest rendered cgroup run = 255
with fgets(buffer, sizeof(buffer)) longest rendered cgroup run = 255
So this is hygiene rather than a user-visible fix, and it can only ever be
exercised behind #2051. That is not an argument against merging it, just a note
in case the commit message wants to be accurate about impact.
One adjacent thing this exposed, clearly out of scope for a one-liner: the loop
has no partial-line handling. When a line does exceed the buffer, the next
fgets returns the remainder of that same line and the parser treats it as a
new cgroup entry, walking it for two : separators that are not there and
appending a separator plus an empty group to output.
LinuxProcessTable_readSmapsFile() in the same file handles exactly this case
with skipEndOfLine(). If anyone cares about >4KB cgroup lines, that is the
real bug in this function, and it would be a separate PR.
2 participants
https://github.com/htop-dev/htop/blob/main/linux/LinuxProcessTable.c#L935-L941
LinuxProcessTable_readCGroupFile() declares an inner buffer of PROC_LINE_LENGTH + 1 bytes but passes PROC_LINE_LENGTH to fgets(), telling it that the buffer is one byte smaller than it actually is