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
Unknown Object (File)
Tue, Nov 26, 7:07 PM
Unknown Object (File)
Tue, Nov 26, 6:55 PM
Unknown Object (File)
Tue, Nov 26, 12:58 PM
Unknown Object (File)
Mon, Nov 25, 10:09 PM
Unknown Object (File)
Sun, Nov 24, 7:30 PM
Unknown Object (File)
Sat, Nov 23, 1:26 AM
Unknown Object (File)
Thu, Nov 21, 5:21 AM
Unknown Object (File)
Tue, Nov 19, 4:21 PM
View All 87 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

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

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

865

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

lib/libc/gen/scandir.c
85
  • 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

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

lib/libc/gen/setmode.c
178

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

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

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

lib/libc/regex/regcomp.c
1403

Delete the unnecessary casts while here?

lib/libc/rpc/getnetconfig.c
633

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

lib/libc/stdio/ungetc.c
83

*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

Delete space between sizeof and ( while here?

310

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

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

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

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

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

lib/libc/stdio/ungetc.c
83

FWIW, this change was inspired by OpenBSD :).

lib/libc/stdlib/getenv.c
276

Will do.

ngie added inline comments.Mar 12 2017, 2:36 AM
lib/libc/gen/setmode.c
178

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
lib/
libc/
gen/
8 lines
7 lines
4 lines
4 lines
iconv/
4 lines
net/
2 lines
regex/
12 lines
rpc/
4 lines
stdio/
2 lines
5 lines
4 lines
stdlib/
8 lines

Diff 26139

View Options

lib/libc/gen/glob.c

Loading...
View Options

lib/libc/gen/scandir.c

Loading...
View Options

lib/libc/gen/setmode.c

Loading...
View Options

lib/libc/gen/wordexp.c

Loading...
View Options

lib/libc/iconv/citrus_esdb.c

Loading...
View Options

lib/libc/net/nsdispatch.c

Loading...
View Options

lib/libc/regex/regcomp.c

Loading...
View Options

lib/libc/rpc/getnetconfig.c

Loading...
View Options

lib/libc/stdio/open_wmemstream.c

Loading...
View Options

lib/libc/stdio/printf-pos.c

Loading...
View Options

lib/libc/stdio/ungetc.c

Loading...
View Options

lib/libc/stdlib/getenv.c

Loading...