Page MenuHomeFreeBSD

Bug 211958 - Boot overflows when reading loader.conf
ClosedPublic

Authored by tsoome on Aug 18 2016, 1:22 PM.

Details

Summary

loader should use snprintf() when printing dynamic strings into command_errbuf.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

tsoome retitled this revision from to Bug 211958 - Boot overflows when reading loader.conf.
tsoome updated this object.
tsoome edited the test plan for this revision. (Show Details)
tsoome added reviewers: allanjude, imp.
imp edited edge metadata.

like it, except for a few nits.

sys/boot/common/boot.c
64 ↗(On Diff #19455)

Super duper nit alert: No space after sizeof here and elsewhere.

sys/boot/common/bootstrap.h
40 ↗(On Diff #19455)

This isn't needed....

sys/boot/common/commands.c
461 ↗(On Diff #19455)

{ not needed here. optional, so i'll let you keep it if you didn't do this on accident.

This revision is now accepted and ready to land.Aug 19 2016, 4:09 AM
sys/boot/common/bootstrap.h
40 ↗(On Diff #19455)

Actually it is. As the actual variable is defined in command.c, other source files do not see it's size and sizeof() could not be used. Confirmed by compiler - can't argue with it;)

sys/boot/common/boot.c
64 ↗(On Diff #19455)

From cstyle(9): "Space after keywords (if, while, for, return, switch)."

Parentheses there mean an example, not an exact list of keywords to put space after, for exact list you wont need parentheses there at all. sizeof is definitely keyword in C, as are quite many other (I haven't counted them, somewhere was list of 32 keywords;)

sys/boot/common/commands.c
461 ↗(On Diff #19455)

yes, I know it is optional, the cstyle does allow this for multi-line statements, and in general, it keeps multi-line statements more readable. Altho, perhaps this case is not *that* obvious;)

sys/boot/common/boot.c
64 ↗(On Diff #19455)

Except sizeof is special and always has been. It's even explicitly documented:

"Casts and sizeof's are not followed by a space. "

sys/boot/common/bootstrap.h
40 ↗(On Diff #19455)

Ah, yes. You're right. Forget I said anything.

sys/boot/common/commands.c
461 ↗(On Diff #19455)

As long as it was intentional and not the result of some intermediate step where you needed them, it's cool. You're changing the code in the general area, in some cases you're making things multi-line (though not multi-statement). It's a reasonable change if done on purpose, which you've done.

sys/boot/common/boot.c
64 ↗(On Diff #19455)

selective reading:D well, you are right there obviously, thanks for pointing it out:)

tsoome edited edge metadata.

cstyle fix.
Updated to revision 304466.

This revision now requires review to proceed.Aug 19 2016, 2:13 PM

cstyle fixed.

allanjude edited edge metadata.

approved for commit

This revision is now accepted and ready to land.Aug 20 2016, 2:51 PM
This revision was automatically updated to reflect the committed changes.

I know this was committed, but I wanted to note I was cool with it and too busy to tick the 'approved' box until now.