Date: Wed, 24 Jan 2018 13:18:56 -0500 From: Pedro Giffuni <pfg@FreeBSD.org> To: cem@freebsd.org Cc: src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r328218 - in head/sys: amd64/amd64 arm/xscale/ixp425 arm64/arm64 cam cam/ctl compat/ndis dev/aacraid dev/advansys dev/ath dev/beri/virtio dev/bnxt dev/bwn dev/ciss dev/cxgbe/crypto dev/... Message-ID: <544fb5e8-b9b9-9f11-553f-4685b75387c7@FreeBSD.org> In-Reply-To: <CAG6CVpVaKvc6w81UjGNV-t_vpkzuLMkZCe-SR10HAWEmBD21AA@mail.gmail.com> References: <201801211542.w0LFgbsp005980@repo.freebsd.org> <CAG6CVpXxuFyHS11rF=NF6bSSkC2=xnDh=WnbK-aWp4sOomrZ7w@mail.gmail.com> <51ff8aef-5660-7857-e4d5-12cdc77bc071@FreeBSD.org> <CAG6CVpVaKvc6w81UjGNV-t_vpkzuLMkZCe-SR10HAWEmBD21AA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On 01/24/18 12:37, Conrad Meyer wrote: > On Tue, Jan 23, 2018 at 11:40 AM, Pedro Giffuni <pfg@freebsd.org> wrote: >> On 23/01/2018 14:08, Conrad Meyer wrote: >>> On Sun, Jan 21, 2018 at 7:42 AM, Pedro F. Giffuni <pfg@freebsd.org> wrote: >>>> Author: pfg >>>> Date: Sun Jan 21 15:42:36 2018 >>>> New Revision: 328218 >>> I'm confused about this change. Wouldn't it be better to remove the >>> annotation/attributes from mallocarray() than to remove the protection >>> against overflow? >> >> Not in my opinion: it would be better to detect such overflows at compile >> time (or through a static analyzer) than to have late notification though >> panics. > Sure, it would be better, but some situations are only detected at > runtime -- hence mallocarray. And occasional use of the annotations > on systems with plenty of RAM would keep the source tree free of > compiler-detectable overflows, which I suspect are incredibly > uncommon. I think the runtime error cases are way more uncommon, specially if the checks are unnecessary. In any case I collected the patch with malloc--> mallocarray changes in PR 225197. Maybe their are useful with fuzzing. >> The blind use of mallocarray(9) is probably a mistake also: we >> shouldn't use it unless there is some real risk of overflow. > I'm not sure I follow that. You normally don't get "tainted" values in the kernel. Most of the allocations in question were variables with very small number multiplied by the size of an int. As Bruce mentioned, we don't do big allocations with malloc(9), at least not the size required to get an unsigned number overflow. In such cases checking for an overflow is a complete waste of time. And then there is also the bug of mallocarray(9) size types not matching malloc(9). >>> (If the compiler is fixed in the future to not use >>> excessive memory with these attributes, they can be conditionalized on >>> compiler version, of course.) >> All in all, the compiler is not provably wrong: it's just using more swap >> space, which is rather inconvenient for small platforms but not necessarily >> wrong. > Seems wrong if it's a noticeable amount. Maybe we could flip the > annotations on or off with a low-ram build knob or something like > that. There is actually no proof that this was related to the attributes. I absolutely dislike the idea of disabling the attributes and even more the idea of adding complex machinery to the system headers to account for such case. Pedro.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?544fb5e8-b9b9-9f11-553f-4685b75387c7>