Page MenuHomeFreeBSD
Differential D9955

Make use of reallocarray(3) in libc.
ClosedPublic
Actions

Authored by pfg on Mar 11 2017, 4:21 AM.
Tags
None
Referenced Files
F101868713: D9955.diff
Mon, Nov 4, 10:59 PM
Unknown Object (File)
Thu, Oct 17, 1:13 AM
Unknown Object (File)
Mon, Oct 14, 12:57 AM
Unknown Object (File)
Fri, Oct 11, 7:06 PM
Unknown Object (File)
Thu, Oct 10, 9:24 AM
Unknown Object (File)
Tue, Oct 8, 7:57 PM
Unknown Object (File)
Mon, Oct 7, 10:22 PM
Unknown Object (File)
Oct 3 2024, 5:13 PM
View All 78 Files
Subscribers
imp

Details

Reviewers
ngie
delphij
Commits
rS315162: libc: provide some bounds-checking through reallocarray(3).
Summary

libc already includes reallocarray(3) so its use is auto-consistent.

Try to use reallocarray(3) when we are using realloc(3) and there is a
multiplication involved. Attempt to use the unsigned nature of the
parameters unless they are a copy of something that is signed.

While here attempt to improve the initialization on some cases-

Diff Detail

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

Event Timeline

pfg created this revision.Mar 11 2017, 4:21 AM
Herald added a subscriber: imp. · View Herald TranscriptMar 11 2017, 4:21 AM
Harbormaster completed remote builds in B7984: Diff 26139.Mar 11 2017, 5:04 AM
ngie accepted this revision.Mar 12 2017, 2:04 AM

LGTM, sans stylistic changes and style fudging. The only blocking part (which was an issue before) is the nitems comment.

lib/libc/gen/glob.c
864 ↗(On Diff #26139)

This comment (before and after) seems unnecessary. The manpages should be documenting this.

865 ↗(On Diff #26139)

The void* cast seems unnecessary, before and after (the type is char**).

lib/libc/gen/scandir.c
85 ↗(On Diff #26139)
  • Please use a different name. This conflicts with the nitems macro in sys/param.h.
  • Why move the initialization down? It doesn't cause any harm leaving it there, and it might inform gcc that the variable is indeed initialized at the right time.
119 ↗(On Diff #26139)

The struct dirent** cast is unnecessary, as is the char* cast.

lib/libc/gen/setmode.c
178 ↗(On Diff #26139)

Why the type change (I kind of understand why, but I want to be sure my assumptions match the intent).

lib/libc/gen/wordexp.c
238 ↗(On Diff #26139)

Delete the extra space between ...OFFS ? and we..., while here?

This would also be a lot more readable if the assignment was done outside the if-statement and if (nmv == NULL) { was done separately.

lib/libc/iconv/citrus_esdb.c
266 ↗(On Diff #26139)

This is ok, but I'd prefer this to be done as part of a separate cleanup commit.

lib/libc/regex/regcomp.c
1403 ↗(On Diff #26139)

Delete the unnecessary casts while here?

lib/libc/rpc/getnetconfig.c
633 ↗(On Diff #26139)

Same comment about the declaration and comparison being split up for readability that I made above.

lib/libc/stdio/ungetc.c
83 ↗(On Diff #26139)

*groan* Someone was being overly clever (just for my own personal edification, I verified this)...

$ python2
Python 2.7.13 (default, Feb 21 2017, 01:53:52)
[GCC 4.2.1 Compatible FreeBSD Clang 3.9.1 (tags/RELEASE_391/final 289601)] on freebsd12
Type "help", "copyright", "credits" or "license" for more information.
>>> 0 << 1
0
>>> 1 << 1
2
>>> 2 << 1
4
>>> 3 << 1
6
>>> 4 << 1
8
>>> 5 << 1
10
>>> -1 << 1
-2
lib/libc/stdlib/getenv.c
276 ↗(On Diff #26139)

Delete space between sizeof and ( while here?

310 ↗(On Diff #26139)

Delete space between sizeof and ( while here?

This revision is now accepted and ready to land.Mar 12 2017, 2:04 AM
pfg added a comment.Mar 12 2017, 2:21 AM

Some notes here b/c Phabricator doesn't let me reply:

I am not sure it is good to remove all the casts from reallocarray: CERT C coding standard seem to have changed its mind.

MEM02-A. Do not cast the return value from malloc().
has been superceeded by:
MEM02-C. Immediately cast the result of a memory allocation function call
into a pointer to the allocated type.

About the spaces after sizeof (), I was respecting the style in the rest of the file, but you are right, it's better to use KNF while we are here.

  • renaming nitems is a good idea.
  • lazy assignment is recommended in our KNF.
pfg added inline comments.Mar 12 2017, 2:33 AM
lib/libc/gen/glob.c
864 ↗(On Diff #26139)

The comment is pre-existing but I think it is a reminder that 'pglob->gl_pathv' can be NULL.

lib/libc/gen/scandir.c
85 ↗(On Diff #26139)

It is better to initialize nitems (or numitems as it will be renamed, thanks) near to where it's used. If '(names ==NULL)' we will jump to 'fail' and avoid the initialization altogether.

lib/libc/gen/setmode.c
178 ↗(On Diff #26139)

setlen is local (basically only used in a macro): unsigning it will give it a bit more space to grow (JIC) and will fit better the unsigned size_t from the reallocarray.

lib/libc/gen/wordexp.c
238 ↗(On Diff #26139)

Yes about the space but changing to an if should be done in a different commit.

lib/libc/stdio/ungetc.c
83 ↗(On Diff #26139)

FWIW, this change was inspired by OpenBSD :).

lib/libc/stdlib/getenv.c
276 ↗(On Diff #26139)

Will do.

ngie added inline comments.Mar 12 2017, 2:36 AM
lib/libc/gen/setmode.c
178 ↗(On Diff #26139)

Ok! Could this be done in a separate supporting commit please?

pfg updated this revision to Diff 26165.Mar 12 2017, 2:59 AM
pfg edited edge metadata.

Update: just for reference ... I will be committing some
cleanups independently as suggested by ngie.

This revision now requires review to proceed.Mar 12 2017, 2:59 AM
Harbormaster failed remote builds in B8001: Diff 26165!Mar 12 2017, 3:35 AM
pfg updated this revision to Diff 26167.Mar 12 2017, 4:01 AM

Forced update to restart the build.

Harbormaster completed remote builds in B8002: Diff 26167.Mar 12 2017, 4:01 AM
ngie added a comment.Mar 12 2017, 4:03 AM
In D9955#205916, @pfg wrote:

Forced update to restart the build.

The issue noted by HarborMaster is breakage with riscv, caused by rS315051. I doubt it's caused by your change.

Harbormaster failed remote builds in B8001: Diff 26165!Mar 12 2017, 4:05 AM
Harbormaster completed remote builds in B8002: Diff 26167.Mar 12 2017, 5:00 AM
Closed by commit rS315162: libc: provide some bounds-checking through reallocarray(3). (authored by pfg). · Explain WhyMar 12 2017, 4:03 PM
This revision was automatically updated to reflect the committed changes.
pfg marked 13 inline comments as done.

Revision Contents
Changeset List

PathSize
head/
lib/
libc/
gen/
8 lines
4 lines
2 lines
4 lines
iconv/
2 lines
net/
2 lines
regex/
12 lines
rpc/
4 lines
stdio/
2 lines
2 lines
4 lines
stdlib/
8 lines

Diff 26189

View Options

head/lib/libc/gen/glob.c

Loading...
View Options

head/lib/libc/gen/scandir.c

Loading...
View Options

head/lib/libc/gen/setmode.c

Loading...
View Options

head/lib/libc/gen/wordexp.c

Loading...
View Options

head/lib/libc/iconv/citrus_esdb.c

Loading...
View Options

head/lib/libc/net/nsdispatch.c

Loading...
View Options

head/lib/libc/regex/regcomp.c

Loading...
View Options

head/lib/libc/rpc/getnetconfig.c

Loading...
View Options

head/lib/libc/stdio/open_wmemstream.c

Loading...
View Options

head/lib/libc/stdio/printf-pos.c

Loading...
View Options

head/lib/libc/stdio/ungetc.c

Loading...
View Options

head/lib/libc/stdlib/getenv.c

Loading...