| Message ID | 48395050-0eab-1253-78be-daa406685ad5@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Nov 17, 2016 at 11:14:26AM -0700, Martin Sebor wrote: > --- a/gcc/fortran/interface.c > +++ b/gcc/fortran/interface.c > @@ -2821,7 +2821,8 @@ compare_actual_formal (gfc_actual_arglist **ap, gfc_formal_arglist *formal, > for (f = formal; f; f = f->next) > n++; > > - new_arg = XALLOCAVEC (gfc_actual_arglist *, n); > + /* Take care not to call alloca with a zero argument. */ > + new_arg = XALLOCAVEC (gfc_actual_arglist *, n + !n); > > for (i = 0; i < n; i++) > new_arg[i] = NULL; Ugh, that is just too ugly. I don't see anything wrong on alloca (0), and we don't rely on those pointers being distinct from other pointers. Jakub
On 11/17/2016 11:24 AM, Jakub Jelinek wrote: > On Thu, Nov 17, 2016 at 11:14:26AM -0700, Martin Sebor wrote: >> --- a/gcc/fortran/interface.c >> +++ b/gcc/fortran/interface.c >> @@ -2821,7 +2821,8 @@ compare_actual_formal (gfc_actual_arglist **ap, gfc_formal_arglist *formal, >> for (f = formal; f; f = f->next) >> n++; >> >> - new_arg = XALLOCAVEC (gfc_actual_arglist *, n); >> + /* Take care not to call alloca with a zero argument. */ >> + new_arg = XALLOCAVEC (gfc_actual_arglist *, n + !n); >> >> for (i = 0; i < n; i++) >> new_arg[i] = NULL; > > Ugh, that is just too ugly. I don't see anything wrong on alloca (0), > and we don't rely on those pointers being distinct from other pointers. On systems where alloca was implemented on top of malloc, alloca (0) would cause collection of alloca'd objects that had gone out of scope. There was a time when you'd find alloca (0); calls sprinkled through GCC on purpose. Jeff
On Thu, Nov 17, 2016 at 01:56:03PM -0700, Jeff Law wrote: > On 11/17/2016 11:24 AM, Jakub Jelinek wrote: > >On Thu, Nov 17, 2016 at 11:14:26AM -0700, Martin Sebor wrote: > >>--- a/gcc/fortran/interface.c > >>+++ b/gcc/fortran/interface.c > >>@@ -2821,7 +2821,8 @@ compare_actual_formal (gfc_actual_arglist **ap, gfc_formal_arglist *formal, > >> for (f = formal; f; f = f->next) > >> n++; > >> > >>- new_arg = XALLOCAVEC (gfc_actual_arglist *, n); > >>+ /* Take care not to call alloca with a zero argument. */ > >>+ new_arg = XALLOCAVEC (gfc_actual_arglist *, n + !n); > >> > >> for (i = 0; i < n; i++) > >> new_arg[i] = NULL; > > > >Ugh, that is just too ugly. I don't see anything wrong on alloca (0), > >and we don't rely on those pointers being distinct from other pointers. > On systems where alloca was implemented on top of malloc, alloca (0) would > cause collection of alloca'd objects that had gone out of scope. Ouch. Do we support any such systems as hosts? If yes, can't we just define XALLOCAVEC etc. to alloca (len + 1) or alloca (len ? len : 1) on those systems and leave the sane hosts as is? Jakub
On 11/17/2016 03:03 PM, Jakub Jelinek wrote: > On Thu, Nov 17, 2016 at 01:56:03PM -0700, Jeff Law wrote: >> On 11/17/2016 11:24 AM, Jakub Jelinek wrote: >>> On Thu, Nov 17, 2016 at 11:14:26AM -0700, Martin Sebor wrote: >>>> --- a/gcc/fortran/interface.c >>>> +++ b/gcc/fortran/interface.c >>>> @@ -2821,7 +2821,8 @@ compare_actual_formal (gfc_actual_arglist **ap, gfc_formal_arglist *formal, >>>> for (f = formal; f; f = f->next) >>>> n++; >>>> >>>> - new_arg = XALLOCAVEC (gfc_actual_arglist *, n); >>>> + /* Take care not to call alloca with a zero argument. */ >>>> + new_arg = XALLOCAVEC (gfc_actual_arglist *, n + !n); >>>> >>>> for (i = 0; i < n; i++) >>>> new_arg[i] = NULL; >>> >>> Ugh, that is just too ugly. I don't see anything wrong on alloca (0), >>> and we don't rely on those pointers being distinct from other pointers. >> On systems where alloca was implemented on top of malloc, alloca (0) would >> cause collection of alloca'd objects that had gone out of scope. > > Ouch. Do we support any such systems as hosts? If yes, can't we just > define XALLOCAVEC etc. to alloca (len + 1) or alloca (len ? len : 1) > on those systems and leave the sane hosts as is? I would guess they're all dead as hosts for building GCC. I was most familiar with hpux, but I'm pretty sure there were others as emacs (IIRC) had a replacement alloca for systems without it as a builtin. They probably all fall into the "retro-computing" bucket these days. Essentially those systems worked by recording all the allocations as well as the frame depth at which they occurred. The next time alloca was called, anything at a deeper depth than the current frame was released. So even if we called alloca (0) unexpectedly, it's not going to cause anything to break. Failing to call alloca (0) could run the system out of heap memory. It's left as an exercise to the reader to ponder how that might happen -- it can and did happen building GCC "in the old days". The point is warning on an alloca (0) may not be as clear cut as it might seem. It's probably a reasonable thing to do on the host, but on a target, which might be embedded and explicitly overriding the builtin alloca, warning on alloca (0) is less of a slam dunk. jeff
On 11/17/2016 03:52 PM, Jeff Law wrote: > On 11/17/2016 03:03 PM, Jakub Jelinek wrote: >> On Thu, Nov 17, 2016 at 01:56:03PM -0700, Jeff Law wrote: >>> On 11/17/2016 11:24 AM, Jakub Jelinek wrote: >>>> On Thu, Nov 17, 2016 at 11:14:26AM -0700, Martin Sebor wrote: >>>>> --- a/gcc/fortran/interface.c >>>>> +++ b/gcc/fortran/interface.c >>>>> @@ -2821,7 +2821,8 @@ compare_actual_formal (gfc_actual_arglist >>>>> **ap, gfc_formal_arglist *formal, >>>>> for (f = formal; f; f = f->next) >>>>> n++; >>>>> >>>>> - new_arg = XALLOCAVEC (gfc_actual_arglist *, n); >>>>> + /* Take care not to call alloca with a zero argument. */ >>>>> + new_arg = XALLOCAVEC (gfc_actual_arglist *, n + !n); >>>>> >>>>> for (i = 0; i < n; i++) >>>>> new_arg[i] = NULL; >>>> >>>> Ugh, that is just too ugly. I don't see anything wrong on alloca (0), >>>> and we don't rely on those pointers being distinct from other pointers. >>> On systems where alloca was implemented on top of malloc, alloca (0) >>> would >>> cause collection of alloca'd objects that had gone out of scope. >> >> Ouch. Do we support any such systems as hosts? If yes, can't we just >> define XALLOCAVEC etc. to alloca (len + 1) or alloca (len ? len : 1) >> on those systems and leave the sane hosts as is? > I would guess they're all dead as hosts for building GCC. I was most > familiar with hpux, but I'm pretty sure there were others as emacs > (IIRC) had a replacement alloca for systems without it as a builtin. > They probably all fall into the "retro-computing" bucket these days. > > Essentially those systems worked by recording all the allocations as > well as the frame depth at which they occurred. The next time alloca > was called, anything at a deeper depth than the current frame was released. > > So even if we called alloca (0) unexpectedly, it's not going to cause > anything to break. Failing to call alloca (0) could run the system out > of heap memory. It's left as an exercise to the reader to ponder how > that might happen -- it can and did happen building GCC "in the old days". > > The point is warning on an alloca (0) may not be as clear cut as it > might seem. It's probably a reasonable thing to do on the host, but on > a target, which might be embedded and explicitly overriding the builtin > alloca, warning on alloca (0) is less of a slam dunk. I confess I haven't heard of such an implementation before but after some searching I have managed to find a few examples of it online, such as in QNX or in the BlackBerry SDK, or in the GCC shipped by Texas Instruments. For instance: http://www.qnx.com/developers/docs/6.5.0SP1.update/com.qnx.doc.neutrino_lib_ref/a/alloca.html Their GCC-derived compilers apparently enable it with -fno-builtins. Other references include source code derived from an implementation with Doug Gwyn's name on it, such as this one: https://opensource.apple.com/source/gnudiff/gnudiff-10/diffutils/alloca.c A comment at the top the file mentions the alloca(0) use case: As a special case, alloca(0) reclaims storage without allocating any. It is a good idea to use alloca(0) in your main control loop, etc. to force garbage collection. That said, I don't view this as reason to exclude the warning from -Wextra. The vendor-provided compilers are obviously customized versions of GCC with their own features, including their own set of options and warnings. It shouldn't stop us from enabling those we expect to be useful to the typical GCC user on typical targets, and especially those that can help expose sources of common bugs. Recognizing I'm preaching to choir here: Calling alloca with any argument is considered dangerous enough to be discouraged in most man pages. Alloca(0) is a potentially dangerous corner case of an already dangerous API. It seems at least as problematic as any of the warnings already in -Wextra, and I'd say as many in -Wall. Even on systems with this unusual alloca implementation, since alloca(0) is special (and expensive), warning on such calls would likely be useful. In fact, since calling alloca(0) in these environments is actually important, warning on them coukd help detect their unintended absence. The few such calls that are intended can be easily tweaked to suppress the warning. Martin
On 11/17/2016 05:32 PM, Martin Sebor wrote: > On 11/17/2016 03:52 PM, Jeff Law wrote: >> On 11/17/2016 03:03 PM, Jakub Jelinek wrote: >>> On Thu, Nov 17, 2016 at 01:56:03PM -0700, Jeff Law wrote: >>>> On 11/17/2016 11:24 AM, Jakub Jelinek wrote: >>>>> On Thu, Nov 17, 2016 at 11:14:26AM -0700, Martin Sebor wrote: >>>>>> --- a/gcc/fortran/interface.c >>>>>> +++ b/gcc/fortran/interface.c >>>>>> @@ -2821,7 +2821,8 @@ compare_actual_formal (gfc_actual_arglist >>>>>> **ap, gfc_formal_arglist *formal, >>>>>> for (f = formal; f; f = f->next) >>>>>> n++; >>>>>> >>>>>> - new_arg = XALLOCAVEC (gfc_actual_arglist *, n); >>>>>> + /* Take care not to call alloca with a zero argument. */ >>>>>> + new_arg = XALLOCAVEC (gfc_actual_arglist *, n + !n); >>>>>> >>>>>> for (i = 0; i < n; i++) >>>>>> new_arg[i] = NULL; >>>>> >>>>> Ugh, that is just too ugly. I don't see anything wrong on alloca (0), >>>>> and we don't rely on those pointers being distinct from other >>>>> pointers. >>>> On systems where alloca was implemented on top of malloc, alloca (0) >>>> would >>>> cause collection of alloca'd objects that had gone out of scope. >>> >>> Ouch. Do we support any such systems as hosts? If yes, can't we just >>> define XALLOCAVEC etc. to alloca (len + 1) or alloca (len ? len : 1) >>> on those systems and leave the sane hosts as is? >> I would guess they're all dead as hosts for building GCC. I was most >> familiar with hpux, but I'm pretty sure there were others as emacs >> (IIRC) had a replacement alloca for systems without it as a builtin. >> They probably all fall into the "retro-computing" bucket these days. >> >> Essentially those systems worked by recording all the allocations as >> well as the frame depth at which they occurred. The next time alloca >> was called, anything at a deeper depth than the current frame was >> released. >> >> So even if we called alloca (0) unexpectedly, it's not going to cause >> anything to break. Failing to call alloca (0) could run the system out >> of heap memory. It's left as an exercise to the reader to ponder how >> that might happen -- it can and did happen building GCC "in the old >> days". >> >> The point is warning on an alloca (0) may not be as clear cut as it >> might seem. It's probably a reasonable thing to do on the host, but on >> a target, which might be embedded and explicitly overriding the builtin >> alloca, warning on alloca (0) is less of a slam dunk. > > I confess I haven't heard of such an implementation before but after > some searching I have managed to find a few examples of it online, > such as in QNX or in the BlackBerry SDK, or in the GCC shipped by > Texas Instruments. For instance: > > http://www.qnx.com/developers/docs/6.5.0SP1.update/com.qnx.doc.neutrino_lib_ref/a/alloca.html > > > Their GCC-derived compilers apparently enable it with -fno-builtins. > > Other references include source code derived from an implementation > with Doug Gwyn's name on it, such as this one: > > https://opensource.apple.com/source/gnudiff/gnudiff-10/diffutils/alloca.c There's also a copy in libiberty: https://gcc.gnu.org/onlinedocs/libiberty/Functions.html#index-alloca-58 https://gcc.gnu.org/viewcvs/gcc/trunk/libiberty/alloca.c?view=markup > > A comment at the top the file mentions the alloca(0) use case: > > As a special case, alloca(0) reclaims storage without > allocating any. It is a good idea to use alloca(0) in > your main control loop, etc. to force garbage collection. > > That said, I don't view this as reason to exclude the warning from > -Wextra. The vendor-provided compilers are obviously customized > versions of GCC with their own features, including their own set > of options and warnings. It shouldn't stop us from enabling those > we expect to be useful to the typical GCC user on typical targets, > and especially those that can help expose sources of common bugs. > Recognizing I'm preaching to choir here: Calling alloca with any > argument is considered dangerous enough to be discouraged in most > man pages. Alloca(0) is a potentially dangerous corner case of > an already dangerous API. It seems at least as problematic as > any of the warnings already in -Wextra, and I'd say as many in > -Wall. > > Even on systems with this unusual alloca implementation, since > alloca(0) is special (and expensive), warning on such calls would > likely be useful. In fact, since calling alloca(0) in these > environments is actually important, warning on them coukd help > detect their unintended absence. The few such calls that are > intended can be easily tweaked to suppress the warning. > > Martin
On Thu, Nov 17, 2016 at 05:32:31PM -0700, Martin Sebor wrote: > >The point is warning on an alloca (0) may not be as clear cut as it > >might seem. It's probably a reasonable thing to do on the host, but on > >a target, which might be embedded and explicitly overriding the builtin > >alloca, warning on alloca (0) is less of a slam dunk. > > I confess I haven't heard of such an implementation before but after > some searching I have managed to find a few examples of it online, > such as in QNX or in the BlackBerry SDK, or in the GCC shipped by > Texas Instruments. For instance: In the libiberty/alloca.c, I don't see anything special about alloca (0). Yes, alloca (0) reclaims garbage, but so does alloca (1) and alloca (4035). alloca (0) just returns NULL after it. The diffutils link is the same alloca.c as in libiberty. Returning NULL or returning a non-unique pointer for alloca (0) is just fine, it is the same thing as returning NULL or returning a non-unique pointer from malloc (0). We definitely don't want to warn for malloc (0), and IMNSHO don't want to warn for alloca (0), because it behaves the same. Jakub
On 11/18/2016 12:58 AM, Jakub Jelinek wrote: > On Thu, Nov 17, 2016 at 05:32:31PM -0700, Martin Sebor wrote: >>> The point is warning on an alloca (0) may not be as clear cut as it >>> might seem. It's probably a reasonable thing to do on the host, but on >>> a target, which might be embedded and explicitly overriding the builtin >>> alloca, warning on alloca (0) is less of a slam dunk. >> >> I confess I haven't heard of such an implementation before but after >> some searching I have managed to find a few examples of it online, >> such as in QNX or in the BlackBerry SDK, or in the GCC shipped by >> Texas Instruments. For instance: > > In the libiberty/alloca.c, I don't see anything special about alloca (0). > Yes, alloca (0) reclaims garbage, but so does alloca (1) and alloca (4035). > alloca (0) just returns NULL after it. The diffutils link is the same > alloca.c as in libiberty. Returning NULL or returning a non-unique pointer > for alloca (0) is just fine, it is the same thing as returning NULL or > returning a non-unique pointer from malloc (0). We definitely don't want > to warn for malloc (0), and IMNSHO don't want to warn for alloca (0), > because it behaves the same. I disagree. At a minimum, the return value of malloc(0) is implementation-defined and so relying on it being either null or non-null is non-portable and can be a source of subtle bugs. But malloc(0) has also been known to result from unsigned overflow which has led to vulnerabilities and exploits (famously by the JPG COM vulnerability exploit, but there are plenty of others, including for instance CVE-2016-2851). Much academic research has been devoted to this problem and to solutions to detect and prevent it. The following paper has some good background and references: https://www.usenix.org/legacy/event/woot10/tech/full_papers/Vanegue.pdf Coding standards rules have been developed requiring conforming software to avoid allocating zero bytes for these reasons. TS 1796, the C Secure Coding Rules technical specification, has such a requirement. It was derived from the CERT C Secure Coding rule MEM04-C. Beware of zero-length allocations: https://www.securecoding.cert.org/confluence/x/GQI The same argument applies to alloca(0) with the added caveat that, unlike with the other allocation functions, a non-null return value need not be distinct. In the absence of a policy or guidelines it's a matter of opinion whether or not this warning belongs in either -Wall or -Wextra. Based on the severity of the problems that allocating zero size has been linked to I think it could be successfully argued that it should be in -Wall (the problems are obviously more serious than those that have ever been associated with the -Wunused-type warnings, for example). I put it in -Wextra only because I was trying to be sensitive to the "no false positive" argument. All this said, before debating the fine details of under which conditions each of the new warninsg should be enabled, I was hoping to get comments on the meat of the patch that implements the warnings. Martin
On Fri, Nov 18, 2016 at 09:21:38AM -0700, Martin Sebor wrote: > >In the libiberty/alloca.c, I don't see anything special about alloca (0). > >Yes, alloca (0) reclaims garbage, but so does alloca (1) and alloca (4035). > >alloca (0) just returns NULL after it. The diffutils link is the same > >alloca.c as in libiberty. Returning NULL or returning a non-unique pointer > >for alloca (0) is just fine, it is the same thing as returning NULL or > >returning a non-unique pointer from malloc (0). We definitely don't want > >to warn for malloc (0), and IMNSHO don't want to warn for alloca (0), > >because it behaves the same. > > I disagree. At a minimum, the return value of malloc(0) is > implementation-defined and so relying on it being either null > or non-null is non-portable and can be a source of subtle bugs. Most apps know what malloc (0) means and treat it correctly, they know they shouldn't dereference the pointer, because it is either NULL or holds an array with 0 elements. I fail to see why you would want to warn. > But malloc(0) has also been known to result from unsigned overflow > which has led to vulnerabilities and exploits (famously by the JPG > COM vulnerability exploit, but there are plenty of others, including > for instance CVE-2016-2851). Much academic research has been devoted > to this problem and to solutions to detect and prevent it. How is it different from overflowing to 1 or 2 or 27? What is special on the value 0? > In the absence of a policy or guidelines it's a matter of opinion > whether or not this warning belongs in either -Wall or -Wextra. It IMHO doesn't belong to either of these. Jakub
On 11/18/16, Jakub Jelinek <jakub@redhat.com> wrote: > On Fri, Nov 18, 2016 at 09:21:38AM -0700, Martin Sebor wrote: >> >In the libiberty/alloca.c, I don't see anything special about alloca >> > (0). >> >Yes, alloca (0) reclaims garbage, but so does alloca (1) and alloca >> > (4035). >> >alloca (0) just returns NULL after it. The diffutils link is the same >> >alloca.c as in libiberty. Returning NULL or returning a non-unique >> > pointer >> >for alloca (0) is just fine, it is the same thing as returning NULL or >> >returning a non-unique pointer from malloc (0). We definitely don't >> > want >> >to warn for malloc (0), and IMNSHO don't want to warn for alloca (0), >> >because it behaves the same. >> >> I disagree. At a minimum, the return value of malloc(0) is >> implementation-defined and so relying on it being either null >> or non-null is non-portable and can be a source of subtle bugs. > > Most apps know what malloc (0) means and treat it correctly, they know they > shouldn't dereference the pointer, because it is either NULL or holds an > array with 0 elements. I fail to see why you would want to warn. > Just as a reference point, my old version of the clang static analyzer warns for malloc(0); but not alloca(0); though. For example: $ cat alloc_0.c #include <alloca.h> #include <stdlib.h> void fn(void) { char *ptr0 = (char *)malloc(0); void *ptr1 = alloca(0); *ptr0 = *(char *)ptr1; } $ clang -Wall -Wextra -pedantic --analyze -c alloc_0.c alloc_0.c:6:23: warning: Call to 'malloc' has an allocation size of 0 bytes char *ptr0 = (char *)malloc(0); ^ ~ 1 warning generated. Doing some more Googling on the topic finds debates as to whether this warning is warranted or not, but it seems like people are pretty used to dealing with it from clang already, so they probably wouldn't mind too much if gcc started being consistent with it.
On 11/18/2016 09:29 AM, Jakub Jelinek wrote: > On Fri, Nov 18, 2016 at 09:21:38AM -0700, Martin Sebor wrote: >>> In the libiberty/alloca.c, I don't see anything special about alloca (0). >>> Yes, alloca (0) reclaims garbage, but so does alloca (1) and alloca (4035). >>> alloca (0) just returns NULL after it. The diffutils link is the same >>> alloca.c as in libiberty. Returning NULL or returning a non-unique pointer >>> for alloca (0) is just fine, it is the same thing as returning NULL or >>> returning a non-unique pointer from malloc (0). We definitely don't want >>> to warn for malloc (0), and IMNSHO don't want to warn for alloca (0), >>> because it behaves the same. >> >> I disagree. At a minimum, the return value of malloc(0) is >> implementation-defined and so relying on it being either null >> or non-null is non-portable and can be a source of subtle bugs. > > Most apps know what malloc (0) means and treat it correctly, they know they > shouldn't dereference the pointer, because it is either NULL or holds an > array with 0 elements. I fail to see why you would want to warn. Because people make mistakes and warnings help us avoid them (isn't that obvious?) Just because we get it right most of the time doesn't mean we get right all of the time. The papers and exploits I pointed to should provide ample evidence that zero allocations are a problem that can have serious (and costly) consequences. We (i.e., Red Hat) recognize this risk and have made it our goal to help stem some of these problems. >> But malloc(0) has also been known to result from unsigned overflow >> which has led to vulnerabilities and exploits (famously by the JPG >> COM vulnerability exploit, but there are plenty of others, including >> for instance CVE-2016-2851). Much academic research has been devoted >> to this problem and to solutions to detect and prevent it. > > How is it different from overflowing to 1 or 2 or 27? What is special on > the value 0? It's a case that, unlike the others, can be readily detected. It would be nice to detect the others as well but GCC can't do that yet. That doesn't mean we shouldn't try to detect at least the small subset for now. It doesn't have to be perfect for it to be useful. > >> In the absence of a policy or guidelines it's a matter of opinion >> whether or not this warning belongs in either -Wall or -Wextra. > > It IMHO doesn't belong to either of these. You've made that quite clear. I struggle to reconcile your position in this case with the one in debate about the -Wimplicit-fallthrough option where you insisted on the exact opposite despite the overwhelming number of false positives caused by it. Why is it appropriate for that option to be in -Wextra and not this one? Martin
On Fri, Nov 18, 2016 at 10:14:09AM -0700, Martin Sebor wrote: > Because people make mistakes and warnings help us avoid them (isn't > that obvious?) Just because we get it right most of the time doesn't > mean we get right all of the time. The papers and exploits I pointed > to should provide ample evidence that zero allocations are a problem > that can have serious (and costly) consequences. We (i.e., Red Hat) > recognize this risk and have made it our goal to help stem some of > these problems. Are you talking about cases where user writes malloc (0) explicitly, or where user writes malloc (n); and the optimizers figure out n is 0, either always, or e.g. because the compiler decided to duplicate the code and and in one of the branches it proves it is 0, or you want to add a runtime warning when malloc is called with 0? E.g. I don't see how writing malloc (0) in the code should have anything to do with any overflows. The cases where jump threading creates cases where we call functions with constant arguments and we then warn on them is also problematic, often such code is even dead except the compiler is not able to prove it. > >It IMHO doesn't belong to either of these. > > You've made that quite clear. I struggle to reconcile your > position in this case with the one in debate about the > -Wimplicit-fallthrough option where you insisted on the exact > opposite despite the overwhelming number of false positives > caused by it. Why is it appropriate for that option to be in > -Wextra and not this one? It also matters what one can do to tell the compiler the code is fine. For e.g. -Wimplicit-fallthrough and many other warnings one can add a comment, or attribute, etc. to get the warning out of the way. But the patch you've posted for the alloca (0) stuff contained changes of n to n + !n, so the warning basically asks people to slow down their code for no reason, just to get the warning out of the way. Jakub
On 11/17/2016 05:32 PM, Martin Sebor wrote: > > I confess I haven't heard of such an implementation before but after > some searching I have managed to find a few examples of it online, > such as in QNX or in the BlackBerry SDK, or in the GCC shipped by > Texas Instruments. For instance: I have the "advantage" of being a GCC gray beard and had the misfortune of maintaining a system that had one of those allocas (hpux) *and* having to debug heap exhaustions in GCC that would occur due to this kind of construct for (some large number of iterations) frobit (...) Where frobit would allocate a metric ton of stuff with alloca. Note the calls to alloca down in frobit would all appear to be at the same stack depth (alloca literally recorded the SP value and the PA was a fixed frame target). So the calls to alloca within frobit wouldn't deallocate anything. > > http://www.qnx.com/developers/docs/6.5.0SP1.update/com.qnx.doc.neutrino_lib_ref/a/alloca.html > > > Their GCC-derived compilers apparently enable it with -fno-builtins. Right. > > That said, I don't view this as reason to exclude the warning from > -Wextra. The vendor-provided compilers are obviously customized > versions of GCC with their own features, including their own set > of options and warnings. It shouldn't stop us from enabling those > we expect to be useful to the typical GCC user on typical targets, > and especially those that can help expose sources of common bugs. > Recognizing I'm preaching to choir here: Calling alloca with any > argument is considered dangerous enough to be discouraged in most > man pages. Alloca(0) is a potentially dangerous corner case of > an already dangerous API. It seems at least as problematic as > any of the warnings already in -Wextra, and I'd say as many in > -Wall. Right. We're well in the "living dangerously" space. But I could claim that on one of these targets, warning about an alloca (0) would likely result in a developer removing it or forcing it to allocate some small amount of space to shut up the warning. That in turn runs the risk of making the target code more prone to heap exhaustion and possibly less secure, particularly if the developer isn't aware of the special nature of alloca (0). > > Even on systems with this unusual alloca implementation, since > alloca(0) is special (and expensive), warning on such calls would > likely be useful. In fact, since calling alloca(0) in these > environments is actually important, warning on them coukd help > detect their unintended absence. The few such calls that are > intended can be easily tweaked to suppress the warning. It certainly is special and often expensive. But I'd come back to the likely behavior of the developer. I suspect unless there's a comment indicating the alloca (0) is intentional, they're likely to just remove it. So I see both sides here and I'm not sure what the best path forward should be. jeff
On 11/18/2016 12:58 AM, Jakub Jelinek wrote: > On Thu, Nov 17, 2016 at 05:32:31PM -0700, Martin Sebor wrote: >>> The point is warning on an alloca (0) may not be as clear cut as it >>> might seem. It's probably a reasonable thing to do on the host, but on >>> a target, which might be embedded and explicitly overriding the builtin >>> alloca, warning on alloca (0) is less of a slam dunk. >> >> I confess I haven't heard of such an implementation before but after >> some searching I have managed to find a few examples of it online, >> such as in QNX or in the BlackBerry SDK, or in the GCC shipped by >> Texas Instruments. For instance: > > In the libiberty/alloca.c, I don't see anything special about alloca (0). > Yes, alloca (0) reclaims garbage, but so does alloca (1) and alloca (4035). > alloca (0) just returns NULL after it. The diffutils link is the same > alloca.c as in libiberty. Returning NULL or returning a non-unique pointer > for alloca (0) is just fine, it is the same thing as returning NULL or > returning a non-unique pointer from malloc (0). We definitely don't want > to warn for malloc (0), and IMNSHO don't want to warn for alloca (0), > because it behaves the same. Maybe that's the key. We don't want to warn for alloca (0); But we should warn for whatever = alloca (0); The former is a clear request for GC of the alloca space. The latter it much more likely a request for space where something went wrong computing the amount of space. jeff
On 11/18/2016 11:46 AM, Jeff Law wrote: > On 11/18/2016 12:58 AM, Jakub Jelinek wrote: >> On Thu, Nov 17, 2016 at 05:32:31PM -0700, Martin Sebor wrote: >>>> The point is warning on an alloca (0) may not be as clear cut as it >>>> might seem. It's probably a reasonable thing to do on the host, but on >>>> a target, which might be embedded and explicitly overriding the builtin >>>> alloca, warning on alloca (0) is less of a slam dunk. >>> >>> I confess I haven't heard of such an implementation before but after >>> some searching I have managed to find a few examples of it online, >>> such as in QNX or in the BlackBerry SDK, or in the GCC shipped by >>> Texas Instruments. For instance: >> >> In the libiberty/alloca.c, I don't see anything special about alloca (0). >> Yes, alloca (0) reclaims garbage, but so does alloca (1) and alloca >> (4035). >> alloca (0) just returns NULL after it. The diffutils link is the same >> alloca.c as in libiberty. Returning NULL or returning a non-unique >> pointer >> for alloca (0) is just fine, it is the same thing as returning NULL or >> returning a non-unique pointer from malloc (0). We definitely don't want >> to warn for malloc (0), and IMNSHO don't want to warn for alloca (0), >> because it behaves the same. > Maybe that's the key. We don't want to warn for > > alloca (0); > > But we should warn for > > whatever = alloca (0); > > > The former is a clear request for GC of the alloca space. The latter it > much more likely a request for space where something went wrong > computing the amount of space. That makes sense to me. In fact, it already works this way, though not thanks to anything I did. GCC optimizes calls to alloca away when their return value isn't used so they don't trigger the warning. With -fno-builtin-alloca (and with the Glibc alloca macro suppressed), a call to alloca(0) does not emit the warning because Glibc alloca isn't declared with attribute alloc_size (or any other attribute except C++ nothrow). Only calls to the built-in whose return value is used do trigger it, whether the argument is a literal zero or the result of constant propagation. Martin
On 11/18/2016 10:25 AM, Jakub Jelinek wrote: > On Fri, Nov 18, 2016 at 10:14:09AM -0700, Martin Sebor wrote: >> Because people make mistakes and warnings help us avoid them (isn't >> that obvious?) Just because we get it right most of the time doesn't >> mean we get right all of the time. The papers and exploits I pointed >> to should provide ample evidence that zero allocations are a problem >> that can have serious (and costly) consequences. We (i.e., Red Hat) >> recognize this risk and have made it our goal to help stem some of >> these problems. > > Are you talking about cases where user writes malloc (0) explicitly, or > where user writes malloc (n); and the optimizers figure out n is 0, > either always, or e.g. because the compiler decided to duplicate the code > and and in one of the branches it proves it is 0, or you want to add a > runtime warning when malloc is called with 0? Both. The existing -Walloca-larger-than warning and the proposed -Walloc-zero warning diagnose both. The non-constant case (i.e., one where the zero is the result of constant propagation) is the more interesting (and dangerous) one but I don't think there is value in trying to distinguish the two. > E.g. I don't see how writing malloc (0) in the code should have anything > to do with any overflows. The cases where jump threading creates cases > where we call functions with constant arguments and we then warn on them > is also problematic, often such code is even dead except the compiler is not > able to prove it. > >>> It IMHO doesn't belong to either of these. >> >> You've made that quite clear. I struggle to reconcile your >> position in this case with the one in debate about the >> -Wimplicit-fallthrough option where you insisted on the exact >> opposite despite the overwhelming number of false positives >> caused by it. Why is it appropriate for that option to be in >> -Wextra and not this one? > > It also matters what one can do to tell the compiler the code is fine. > For e.g. -Wimplicit-fallthrough and many other warnings one can add > a comment, or attribute, etc. to get the warning out of the way. > But the patch you've posted for the alloca (0) stuff contained > changes of n to n + !n, so the warning basically asks people to slow > down their code for no reason, just to get the warning out of the way. No, it does not. There are ways to avoid the warning without changing the value of the argument. The obvious one is to set -Wno-alloc-zero, either on the command line or via pragma GCC diagnostic. Another one is to call the alloca function instead of the builtin (the function is not declared without attribute alloc_size, at least in Glibc, but GCC still expands it inline). This is as simple as enclosing alloca in parentheses: void *p = (alloca)(n); Ugly? Perhaps. One might say that code that does tricky or unportable things is ugly for that reason alone. IMO, it's a good thing when it also looks unusual (or even ugly). It draws attention to itself and is less likely to be copied or reused, or broken by those who don't realize that it's fragile. The specific patch we're discussing touches just 5 functions in all of GCC, and it changes an alloca(n) to alloca(n + !n). Your original objection was that the fix was too ugly. I'd be happy to change it to (n + 1) if that makes it less offensive to you (or to anything else that lets us move forward, including the (alloca)(n) above). Either way, none of these calls is in hot loops so the runtime impact of any of this on GCC hardly seems worth talking about. Having said that, after another review of the functions changed by the patch it looks like avoiding the zero case (e.g., by returning early or branching) might actually improve their runtime performance (though I doubt that would be worth the effort either). The same could well be true for other such instances of the warning. Martin PS I resisted changing the XALLOCAVEC macro itself but I'm not opposed to it and it's also a possibility.
On 11/18/2016 10:25 AM, Jakub Jelinek wrote: > On Fri, Nov 18, 2016 at 10:14:09AM -0700, Martin Sebor wrote: >> Because people make mistakes and warnings help us avoid them (isn't >> that obvious?) Just because we get it right most of the time doesn't >> mean we get right all of the time. The papers and exploits I pointed >> to should provide ample evidence that zero allocations are a problem >> that can have serious (and costly) consequences. We (i.e., Red Hat) >> recognize this risk and have made it our goal to help stem some of >> these problems. > > Are you talking about cases where user writes malloc (0) explicitly, or > where user writes malloc (n); and the optimizers figure out n is 0, > either always, or e.g. because the compiler decided to duplicate the code > and and in one of the branches it proves it is 0, or you want to add a > runtime warning when malloc is called with 0? > E.g. I don't see how writing malloc (0) in the code should have anything > to do with any overflows. The cases where jump threading creates cases > where we call functions with constant arguments and we then warn on them > is also problematic, often such code is even dead except the compiler is not > able to prove it. I would strongly suggest against using explicit vs implicit for this kind of thing. Various transformations can take an implicit and turn it into an explicit. Various transformations may take what starts off as a complex expression and eventually through cse, cprop, etc the expression collapses down to a 0 which could be explicit or implicit. I believe we should be warning on trying to allocation 0 bytes of memory via malloc, realloc or alloca, with the exception of a non-builtin alloca with no return value, but I think we've covered that elsewhere and Martin's code will DTRT more by accident than design. Are there going to be false positives, probably. But I think Martin has already shown the false positive rate to be far lower than many other warnings. Are there cases where someone might explicitly do malloc (0) and do so in a safe, but potentially non-portable manner. Absolutely. But that doesn't mean we should cater to that particular case. > >>> It IMHO doesn't belong to either of these. >> >> You've made that quite clear. I struggle to reconcile your >> position in this case with the one in debate about the >> -Wimplicit-fallthrough option where you insisted on the exact >> opposite despite the overwhelming number of false positives >> caused by it. Why is it appropriate for that option to be in >> -Wextra and not this one? > > It also matters what one can do to tell the compiler the code is fine. > For e.g. -Wimplicit-fallthrough and many other warnings one can add > a comment, or attribute, etc. to get the warning out of the way. > But the patch you've posted for the alloca (0) stuff contained > changes of n to n + !n, so the warning basically asks people to slow > down their code for no reason, just to get the warning out of the way. I'm not buying that as a valid argument. I suspect the amount of computing power to shuttle around the messages on this topic already outweighs the computing power for all the + !ns that might get added to code to avoid this warning. Note that folks routinely have put initializers in code to avoid false positives from our uninitialized variables. IMHO, the biggest argument against the + !n is that it makes the code less clear. I think a simple assertion is a better choice. It clearly documents that zero is not expected, stops the program if it does indeed occur and can be optimized away just as effectively as +!n when n is known to be nonzero. Jeff
On 11/18/2016 10:14 AM, Martin Sebor wrote: >> >> Most apps know what malloc (0) means and treat it correctly, they know >> they >> shouldn't dereference the pointer, because it is either NULL or holds an >> array with 0 elements. I fail to see why you would want to warn. > > Because people make mistakes and warnings help us avoid them (isn't > that obvious?) Just because we get it right most of the time doesn't > mean we get right all of the time. The papers and exploits I pointed > to should provide ample evidence that zero allocations are a problem > that can have serious (and costly) consequences. We (i.e., Red Hat) > recognize this risk and have made it our goal to help stem some of > these problems. I suspect most applications don't ever do malloc (0) and that its appearance is more likely an indicator of a bug. That's what I want to cater to -- finding bugs before they get out into the field. For the oddball application that wants to malloc (0) and try to do something sensible, they can turn off the warning. So I'm in agreement with Martin here. > >>> But malloc(0) has also been known to result from unsigned overflow >>> which has led to vulnerabilities and exploits (famously by the JPG >>> COM vulnerability exploit, but there are plenty of others, including >>> for instance CVE-2016-2851). Much academic research has been devoted >>> to this problem and to solutions to detect and prevent it. >> >> How is it different from overflowing to 1 or 2 or 27? What is special on >> the value 0? > > It's a case that, unlike the others, can be readily detected. It > would be nice to detect the others as well but GCC can't do that > yet. That doesn't mean we shouldn't try to detect at least the > small subset for now. It doesn't have to be perfect for it to be > useful. Right. And as I know I've mentioned to some folks, I'd really like us to be pondering a "may overflow" warning for expressions that feed into an allocator. There's a lot of value in that, but I suspect a lot of noise as well. > >> >>> In the absence of a policy or guidelines it's a matter of opinion >>> whether or not this warning belongs in either -Wall or -Wextra. >> >> It IMHO doesn't belong to either of these. > > You've made that quite clear. I struggle to reconcile your > position in this case with the one in debate about the > -Wimplicit-fallthrough option where you insisted on the exact > opposite despite the overwhelming number of false positives > caused by it. Why is it appropriate for that option to be in > -Wextra and not this one? I disagree with Jakub on this. I think the warning would be fine for Wextra. It's a lot less invasive than other stuff that's gone in there. jeff
On Wed, Nov 23, 2016 at 02:27:05PM -0700, Jeff Law wrote: > I believe we should be warning on trying to allocation 0 bytes of memory via > malloc, realloc or alloca, with the exception of a non-builtin alloca with > no return value, but I think we've covered that elsewhere and Martin's code > will DTRT more by accident than design. But we aren't going to warn on all places which might call alloca at runtime with 0 arguments, you would need runtime instrumentation for that (like ubsan). You are going to warn about explicit cases and random subset of the other ones where the user is just unlucky enough that the compiler threaded some jumps etc. For the explicit ptr = alloca (0) it is easy to get rid of it, for the implicit one one has to pessimize code if they want to avoid the warning. Jakub
On 11/24/2016 12:47 AM, Jakub Jelinek wrote: > On Wed, Nov 23, 2016 at 02:27:05PM -0700, Jeff Law wrote: >> I believe we should be warning on trying to allocation 0 bytes of memory via >> malloc, realloc or alloca, with the exception of a non-builtin alloca with >> no return value, but I think we've covered that elsewhere and Martin's code >> will DTRT more by accident than design. > > But we aren't going to warn on all places which might call alloca at runtime > with 0 arguments, you would need runtime instrumentation for that (like > ubsan). You are going to warn about explicit cases and random subset of the > other ones where the user is just unlucky enough that the compiler threaded > some jumps etc. For the explicit ptr = alloca (0) it is easy to get rid of > it, for the implicit one one has to pessimize code if they want to avoid the > warning. No, it's a compile-time warning when the 0 size argument can be exposed. It's not unlike many other warnings in GCC. jeff
gcc/fortran/ChangeLog: * interface.c (compare_actual_formal): Avoid calling alloca with an argument of zero. * symbol.c (do_traverse_symtree): Same. * trans-intrinsic.c (gfc_conv_intrinsic_ishftc): Same. gcc/ChangeLog: * reg-stack.c (subst_asm_stack_regs): Same. * tree-ssa-threadedge.c (record_temporary_equivalences_from_stmts_at_dest): Same. diff --git a/gcc/fortran/interface.c b/gcc/fortran/interface.c index a0cb0bb..2da51d7 100644 --- a/gcc/fortran/interface.c +++ b/gcc/fortran/interface.c @@ -2821,7 +2821,8 @@ compare_actual_formal (gfc_actual_arglist **ap, gfc_formal_arglist *formal, for (f = formal; f; f = f->next) n++; - new_arg = XALLOCAVEC (gfc_actual_arglist *, n); + /* Take care not to call alloca with a zero argument. */ + new_arg = XALLOCAVEC (gfc_actual_arglist *, n + !n); for (i = 0; i < n; i++) new_arg[i] = NULL; diff --git a/gcc/fortran/symbol.c b/gcc/fortran/symbol.c index 0b711ca..cf403c4 100644 --- a/gcc/fortran/symbol.c +++ b/gcc/fortran/symbol.c @@ -3979,7 +3979,8 @@ do_traverse_symtree (gfc_symtree *st, void (*st_func) (gfc_symtree *), gcc_assert ((st_func && !sym_func) || (!st_func && sym_func)); nodes = count_st_nodes (st); - st_vec = XALLOCAVEC (gfc_symtree *, nodes); + /* Take care not to call alloca with a zero argument. */ + st_vec = XALLOCAVEC (gfc_symtree *, nodes + !nodes); node_cntr = 0; fill_st_vector (st, st_vec, node_cntr); diff --git a/gcc/fortran/trans-intrinsic.c b/gcc/fortran/trans-intrinsic.c index 463bb58..e4715f8 100644 --- a/gcc/fortran/trans-intrinsic.c +++ b/gcc/fortran/trans-intrinsic.c @@ -5525,7 +5525,8 @@ gfc_conv_intrinsic_ishftc (gfc_se * se, gfc_expr * expr) unsigned int num_args; num_args = gfc_intrinsic_argument_list_length (expr); - args = XALLOCAVEC (tree, num_args); + /* Take care not to call alloca with a zero argument. */ + args = XALLOCAVEC (tree, num_args + !num_args); gfc_conv_intrinsic_function_args (se, expr, args, num_args); diff --git a/gcc/reg-stack.c b/gcc/reg-stack.c index 4e86fa9..7da5a5f 100644 --- a/gcc/reg-stack.c +++ b/gcc/reg-stack.c @@ -2050,9 +2050,10 @@ subst_asm_stack_regs (rtx_insn *insn, stack_ptr regstack) for (i = 0, note = REG_NOTES (insn); note; note = XEXP (note, 1)) i++; - note_reg = XALLOCAVEC (rtx, i); - note_loc = XALLOCAVEC (rtx *, i); - note_kind = XALLOCAVEC (enum reg_note, i); + /* Take care not to call alloca with a zero argument. */ + note_reg = XALLOCAVEC (rtx, i + !i); + note_loc = XALLOCAVEC (rtx *, i + !i); + note_kind = XALLOCAVEC (enum reg_note, i + !i); n_notes = 0; for (note = REG_NOTES (insn); note; note = XEXP (note, 1)) diff --git a/gcc/tree-ssa-threadedge.c b/gcc/tree-ssa-threadedge.c index 534292c..0564e69 100644 --- a/gcc/tree-ssa-threadedge.c +++ b/gcc/tree-ssa-threadedge.c @@ -340,7 +340,8 @@ record_temporary_equivalences_from_stmts_at_dest (edge e, unsigned int num, i = 0; num = NUM_SSA_OPERANDS (stmt, SSA_OP_ALL_USES); - copy = XALLOCAVEC (tree, num); + /* Take care not to call alloca with a zero argument. */ + copy = XALLOCAVEC (tree, num + !num); /* Make a copy of the uses & vuses into USES_COPY, then cprop into the operands. */