Use of reallocarray(3) in userland.

[pfg]

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.

[imp]

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]

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?

[pfg]

In D9915#205626, @delphij wrote:

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]

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]

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

[pfg]

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

[pfg]

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