Page MenuHomeFreeBSD
Differential D6490

Fix CID 1006692 in /usr/sbin/pw pw_log() function and other fixes
ClosedPublic
Actions

Authored by truckman on May 21 2016, 11:08 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 8, 5:14 AM
Unknown Object (File)
Tue, Nov 5, 8:19 AM
Unknown Object (File)
Sat, Oct 26, 1:14 AM
Unknown Object (File)
Fri, Oct 18, 10:08 AM
Unknown Object (File)
Oct 5 2024, 3:29 AM
Unknown Object (File)
Oct 5 2024, 2:25 AM
Unknown Object (File)
Oct 4 2024, 11:34 AM
Unknown Object (File)
Oct 4 2024, 11:00 AM
View All Files
Subscribers
imp

Details

Reviewers
markj
jmallett
delphij
bapt
bdrewery
Commits
rS301557: MFC r300564
rS300564: Fix CID 1006692 in /usr/sbin/pw pw_log() function and other fixes
Summary

Limit the length of the name retrieved from the environment
variable $LOGNAME or $USER. Sanitize it by removing whitespace
to benefit log file parsers. Double any embedded '%' characters
since this string will be embedded in a *printf format string.

Detect and warn about overflow when building the format string.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

truckman updated this revision to Diff 16681.May 21 2016, 11:08 PM
truckman retitled this revision from to Fix CID 1006692 in /usr/sbin/pw pw_log() function and other fixes.
truckman updated this object.
truckman edited the test plan for this revision. (Show Details)
truckman added a reviewer: jmallett.
Herald added a subscriber: imp. · View Herald TranscriptMay 21 2016, 11:08 PM
truckman added reviewers: bapt, markj, delphij, bdrewery.May 22 2016, 12:58 AM
markj accepted this revision.May 24 2016, 1:46 AM
markj edited edge metadata.

I think this looks right from a correctness perspective. It might be nice to have a comment explaining what sanitization we're doing. The amount of indentation in this function could be reduced if we return early in the logfile == NULL case.

usr.sbin/pw/pw_log.c
58 ↗(On Diff #16681)

The indentation looks wonky here, but that may just be phabricator.

67 ↗(On Diff #16681)

The cast seems superfluous since we're comparing with a constant whose value is clearly representable with an int. Does clang/coverity warn about this?

(ditto below)

69 ↗(On Diff #16681)

Why not have cp++ in the loop header?

This revision is now accepted and ready to land.May 24 2016, 1:46 AM
truckman added a comment.May 24 2016, 2:33 AM

I like the early return idea, though it will make the diff a lot bigger.

usr.sbin/pw/pw_log.c
58 ↗(On Diff #16681)

I tried to match the existing style here with outdented "*" for pointers.

67 ↗(On Diff #16681)

Yes, clang complains, which I agree is bogus in this case:

cc -O2 -pipe -g -MD -MF.depend.pw_log.o -MTpw_log.o -std=gnu99 -fstack-protector-strong -Wsystem-headers -Werror -Wall -Wno-format-y2k -W -Wno-unused-parameter -Wstrict-prototypes -Wmissing-prototypes -Wpointer-arith -Wno-uninitialized -Wno-pointer-sign -Wno-empty-body -Wno-string-plus-int -Wno-unused-const-variable -Wno-tautological-compare -Wno-unused-value -Wno-parentheses-equality -Wno-unused-function -Wno-enum-conversion -Wno-unused-local-typedef -Qunused-arguments -c /usr/src/usr.sbin/pw/pw_log.c -o pw_log.o
/usr/src/usr.sbin/pw/pw_log.c:67:11: error: comparison of integers of different

signs: 'int' and 'unsigned long' [-Werror,-Wsign-compare]
                              i < sizeof(sname) - 1; ) {
                              ~ ^ ~~~~~~~~~~~~~~~~~

1 error generated.

69 ↗(On Diff #16681)

Good idea.

truckman updated this revision to Diff 16767.May 24 2016, 2:54 AM
truckman edited edge metadata.

Return early when possible to reduce code nesting.

Add a comment to explain the sanitization of name.

Increment cp at the top of the for loop.

Adjust variable declarations to be more style compliant, including
moving initialization out of the declarations.

This revision now requires review to proceed.May 24 2016, 2:54 AM
truckman updated this revision to Diff 16773.May 24 2016, 4:07 AM
truckman edited edge metadata.

Re-order the tests in the name sanitization loop to eliminate the else clause

truckman updated this revision to Diff 16774.May 24 2016, 4:09 AM

Whitespace changes

markj accepted this revision.May 24 2016, 4:39 AM
markj edited edge metadata.

LGTM, thanks! My only other nit is that it might be nice to include the errant username in the overflow warning message.

This revision is now accepted and ready to land.May 24 2016, 4:39 AM
Closed by commit rS300564: Fix CID 1006692 in /usr/sbin/pw pw_log() function and other fixes (authored by truckman). · Explain WhyMay 24 2016, 5:02 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
head/
usr.sbin/
pw/
94 lines

Diff 16775

View Options

head/usr.sbin/pw/pw_log.c

Loading...