iconvctl(3): remove superfluous NULL pointer tests
ClosedPublic
Actions

Authored by vangyzen on May 12 2016, 8:23 PM.

Details

Reviewers

ngie
bdrewery

Commits

rS299704: iconvctl(3): remove superfluous NULL pointer tests

Summary

convname and dst are guaranteed to be non-NULL by iconv_open(3).
src is an array. Remove these tests for NULL pointers.
While I'm here, eliminate a strlcpy with a correct but suspicious-looking
calculation for the third parameter (i.e. not a simple sizeof).
Compare the strings in-place instead of copying.

Found by: bdrewery
Found by: Coverity
CID: 1130050, 1130056
Sponsored by: Dell Inc.

Test Plan

kyua test -k /usr/tests/lib/libc/iconv/Kyuafile

iconvctl_test:iconvctl_trivialp_test -> passed [0.012s]

Results file id is usr_tests_lib_libc_iconv.20160512-202008-374822
Results saved to /root/.kyua/store/results.usr_tests_lib_libc_iconv.20160512-202008-374822.db

1/1 passed (0 failed)

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

vangyzen updated this revision to Diff 16265.May 12 2016, 8:23 PM

vangyzen retitled this revision from to iconvctl(3): remove superfluous NULL pointer tests.

vangyzen updated this object.

vangyzen edited the test plan for this revision. (Show Details)

vangyzen added reviewers: bdrewery, ngie.

Herald added a subscriber: imp. · View Herald TranscriptMay 12 2016, 8:23 PM

vangyzen updated this object.May 12 2016, 8:29 PM

The testcase looks good.

lib/libc/iconv/bsd_iconv.c
271 ↗	(On Diff #16265)	screen should be sorted last per style(9).
271 ↗	(On Diff #16265)	*srclen
286 ↗	(On Diff #16265)	Does it make sense that this can ever return NULL? The code below seems to suggest it in part, but doesn't do a very good job when it increments dst.

vangyzen added inline comments.May 13 2016, 1:29 PM

lib/libc/iconv/bsd_iconv.c
286 ↗	(On Diff #16265)	iconv_open() guarantees that ci_convname will contain a '/'. See citrus_iconv.c line 213. I imagine the former (and broken) test for a NULL dst was just excessively defensive programming.

Adhere to style(9).

Closed by commit rS299704: iconvctl(3): remove superfluous NULL pointer tests (authored by vangyzen). · Explain WhyMay 14 2016, 12:35 AM

This revision was automatically updated to reflect the committed changes.

vangyzen marked 2 inline comments as done.

ngie added inline comments.May 14 2016, 1:36 AM

lib/libc/iconv/bsd_iconv.c
286 ↗	(On Diff #16265)	Please note this caveat... I'd hate for this to change in the future, and for things to "not work". Maybe some tests should be created around iconv_open to ensure that this invariant is always upheld.