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.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Oct 29, 12:10 PM
Unknown Object (File)
Sun, Oct 27, 12:47 AM
Unknown Object (File)
Oct 4 2024, 11:27 PM
Unknown Object (File)
Oct 4 2024, 4:00 AM
Unknown Object (File)
Oct 4 2024, 1:57 AM
Unknown Object (File)
Oct 3 2024, 7:16 PM
Unknown Object (File)
Sep 21 2024, 10:13 PM
Unknown Object (File)
Sep 20 2024, 12:05 PM
Subscribers

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 - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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 - subversion.
imp added a reviewer: imp.
This revision is now accepted and ready to land.Nov 16 2016, 3:13 PM
lstewart removed a reviewer: ian.
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?

Update diff post r322614 commit.

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.

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 edited edge metadata.

yoda be gone

This revision now requires review to proceed.Aug 18 2017, 12:31 AM
This revision was automatically updated to reflect the committed changes.