Page MenuHomeFreeBSD

Fix an off-by-one error in libsbuf's userland sbuf_vprintf() function.
ClosedPublic

Authored by lstewart on Nov 16 2016, 8:42 AM.

Details

Summary

An off-by-one error exists in sbuf_vprintf()'s use of SBUF_HASROOM() when an sbuf is filled to capacity by vsnprintf(), the loop exits without error, and the sbuf is not marked as auto-extendable.

SBUF_HASROOM() evaluates true if there is room for one or more non-NULL characters, but in the case that the sbuf was filled exactly to capacity, SBUF_HASROOM() evaluates false. Consequently, sbuf_vprintf() incorrectly assigns an ENOMEM error to the sbuf when in fact everything is fine, in turn poisoning the buffer for all subsequent operations.

Correct by moving the ENOMEM assignment into the loop where it can be made unambiguously.

As a related safety net change, explicitly check for the zero bytes drained case in sbuf_drain() and set EDEADLK as the error. This avoids an infinite loop in sbuf_vprintf() if a drain function were to inadvertently return a value of zero to sbuf_drain().

Sponsored by: Netflix, Inc.

Diff Detail

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

Event Timeline

lstewart updated this revision to Diff 22247.Nov 16 2016, 8:42 AM
lstewart retitled this revision from to Fix an off-by-one error in libsbuf's userland sbuf_vprintf() function..
lstewart updated this object.
lstewart edited the test plan for this revision. (Show Details)
lstewart added a reviewer: ian.
lstewart set the repository for this revision to rS FreeBSD src repository.
imp accepted this revision.Nov 16 2016, 3:13 PM
imp added a reviewer: imp.
This revision is now accepted and ready to land.Nov 16 2016, 3:13 PM
lstewart edited the summary of this revision. (Show Details)Aug 8 2017, 12:21 AM
lstewart removed a reviewer: ian.
lstewart updated this revision to Diff 31736.Aug 8 2017, 12:45 AM
lstewart edited edge metadata.

The off-by-one error was incorrectly attributed to the condition that checks if vsnprintf() was successful at rendering all of the specified content into the sbuf.

The original logic is in fact correct, and the off-by-one is actually in sbuf_vprintf()'s use of SBUF_HASROOM() when an sbuf is filled to capacity by vsnprintf(), the loop exits without error, and the sbuf is not marked as auto-extendable.

This revision now requires review to proceed.Aug 8 2017, 12:45 AM

Conrad: would you be willing to sanity check this sbuf change for me as well?

lstewart updated this revision to Diff 32160.Aug 17 2017, 8:06 AM

Update diff post r322614 commit.

cem accepted this revision.Aug 17 2017, 4:44 PM

Looks sane to me :-).

sys/kern/subr_sbuf.c
644 ↗(On Diff #32160)

style nit: != 0. (Style(9) suggests using non-yoda style constant comparisons, by example.)

This revision is now accepted and ready to land.Aug 17 2017, 4:44 PM

@cem Thanks!

sys/kern/subr_sbuf.c
644 ↗(On Diff #32160)

heh, defensive programming habit of mine when comparing constants against variables to avoid unintentional "var = const" assignment type mistakes, but doesn't apply in this case. Will change on commit.

cem added inline comments.Aug 17 2017, 11:33 PM
sys/kern/subr_sbuf.c
644 ↗(On Diff #32160)

Yeah, I get it. Compilers (gcc and clang) can detect this kind of accidental assignment in condition expressions even without the backwards order, with -Wparentheses (part of -Wall).

lstewart updated this revision to Diff 32188.Aug 18 2017, 12:31 AM
lstewart edited edge metadata.

yoda be gone

This revision now requires review to proceed.Aug 18 2017, 12:31 AM
lstewart marked an inline comment as done.Aug 18 2017, 12:31 AM
This revision was automatically updated to reflect the committed changes.