Page MenuHomeFreeBSD

Use of reallocarray(3) in userland.
AbandonedPublic

Authored by pfg on Mar 7 2017, 1:37 AM.

Details

Summary

(This is not meant to be the proposed commit log)
We brought allocarray(3) from OpenBSD for FreeBSD 11. I will admit I was
never a huge fan of it and it makes things less portable to other OSs, but
given the support is already here, I do think we should make the best
possible use of it.

There are some advantages, other than the theoretical overflow prevention:
static checkers benefit from the extra attributes we have defined in our
headers and it somewhat easier to read to have the arguments of the
multiplication separated.

I looked through opengrok and then I crosschecked some of my changes with
OpenBSD. I don't have any intention of adopting the changes everywhere. Also, dueto compiler errors, it seems particularly important to make sure the parameters are unsigned.

Test Plan

Updated tinderbox but crossreading is always helpful.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8851
Build 9229: CI src build
Build 9228: arc lint + arc unit

Event Timeline

pfg updated this revision to Diff 26050.Mar 7 2017, 1:37 AM
pfg retitled this revision from to Extensive use of reallocarray(3) in userland..
pfg updated this object.
pfg edited the test plan for this revision. (Show Details)
pfg added reviewers: delphij, bapt, araujo, ngie.
imp added a comment.EditedMar 10 2017, 5:06 PM

I'm agnostic to the change.
I get why they are doing malloc(a * b) -> reallocarray(NULL, a, b), but wonder if it would be better to have a wider API that's mallocarray(a,b) if you are looking for syntactic goodness along with your overflow protection....

delphij requested changes to this revision.Mar 10 2017, 6:29 PM

Please do not commit as-is.

I think reallocarray() have some benefits, but it still need to be used carefully and it's not a cure of cancer and when doing so, it's better to conduct an audit of surrounding code, or we are just giving us some false sense of safety.

Could you please split this into smaller and reviewable pieces (e.g. one per module, or one per a few modules) so they can be looked closer, by the way? I'm asking for this because when presented with a thousand lines of diff people would tend to say "fine, someone else must have looked at this very carefully" rather than actually looking, and when making comments, we can easily lose track for it if the change is revised, and more importantly large changesets are usually for one logical change across different modules, like refactors, while this is actually trying to address same type of issues across multiple modules.

usr.sbin/uhsoctl/uhsoctl.c
1120 ↗(On Diff #26050)

What if list_size reaches INTMAX?

This revision now requires changes to proceed.Mar 10 2017, 6:29 PM
pfg abandoned this revision.Mar 10 2017, 7:14 PM

Please do not commit as-is.

Indeed, that was not the plan.

I think reallocarray() have some benefits, but it still need to be used carefully and it's not a cure of cancer and when doing so, it's better to conduct an audit of surrounding code, or we are just giving us some false sense of safety.
Could you please split this into smaller and reviewable pieces (e.g. one per module, or one per a few modules) so they can be looked closer, by the way? I'm asking for this because when presented with a thousand lines of diff people would tend to say "fine, someone else must have looked at this very carefully" rather than actually looking, and when making comments, we can easily lose track for it if the change is revised, and more importantly large changesets are usually for one logical change across different modules, like refactors, while this is actually trying to address same type of issues across multiple modules.

That's reasonable, yes. I would like to have it by areas: I will start with the libc changes.

usr.sbin/uhsoctl/uhsoctl.c
1120 ↗(On Diff #26050)

reallocarray() handles size_t on each argument, so it has better chances of detecting overflows there than the realloc had.

It doesn't look like there is any chance that list_size reaches INTMAX but it does look like we can unsign it to further extend the limits.

pfg updated this revision to Diff 27614.Apr 21 2017, 7:44 PM
pfg edited edge metadata.

Resurrect this Differential Revision with a much reduced scope.

Due to some bug, clang 4.0 can sometimes generate wrong code if the
arguments are signed. In general it also happens that the allocation
arguments are so small that using reallocarray() is a waste of time.

Thi is a reduced subset of the previous changes. The mayor changes to libc
and the systems libraries have already been independently reviewed and
committed.

The changes to fsck_ffs and pmcstat are meant to be committed
independently as they include some other adjustments.

pfg updated this revision to Diff 27615.Apr 21 2017, 7:49 PM

Oops .. drop the change in route6d, it's not likely to overflow.

pfg updated this revision to Diff 27637.Apr 22 2017, 4:04 PM

Since fsck_ffs and pmcstat are done, the remaining cases are trivial.

pfg retitled this revision from Extensive use of reallocarray(3) in userland. to Use of reallocarray(3) in userland..Apr 22 2017, 5:01 PM
pfg edited the summary of this revision. (Show Details)
pfg removed a reviewer: gnn.
pfg abandoned this revision.Apr 27 2019, 2:41 PM

Most of these don't make sense as they cant overflow, and those that do were committed.