Re: [PATCH] avoid calling alloca(0)

[Eric Gallager]

On 12/2/16, Bernd Edlinger  wrote:
> On 12/01/16 19:39, Jeff Law wrote:
>> On 11/30/2016 09:09 PM, Martin Sebor wrote:
 What I think this tells us is that we're not at a place where we're
 clean.  But we can incrementally get there.  The warning is only
 catching a fairly small subset of the cases AFAICT.  That's not unusual
 and analyzing why it didn't trigger on those cases might be useful as
 well.
>>>
>>> The warning has no smarts.  It relies on constant propagation and
>>> won't find a call unless it sees it's being made with a constant
>>> zero.  Looking at the top two on the list the calls are in extern
>>> functions not called from the same source file, so it probably just
>>> doesn't see that the functions are being called from another file
>>> with a zero.  Building GCC with LTO might perhaps help.
>> Right.  This is consistent with the limitations of other similar
>> warnings such as null pointer dereferences.
>>
>>>
 So where does this leave us for gcc-7?  I'm wondering if we drop the
 warning in, but not enable it by default anywhere.  We fix the cases we
 can (such as reg-stack,c tree-ssa-threadedge.c, maybe others) before
 stage3 closes, and shoot for the rest in gcc-8, including improvign the
 warning (if there's something we can clearly improve), and enabling the
 warning in -Wall or -Wextra.
>>>
>>> I'm fine with deferring the GCC fixes and working on the cleanup
>>> over time but I don't think that needs to gate enabling the option
>>> with -Wextra.  The warnings can be suppressed or prevented from
>>> causing errors during a GCC build either via a command line option
>>> or by pragma in the code.  AFAICT, from the other warnings I see
>>> go by, this is what has been done for -Wno-implicit-fallthrough
>>> while those warnings are being cleaned up.  Why not take the same
>>> approach here?
>> The difference vs implicit fallthrough is that new instances of implicit
>> fallthrus aren't likely to be exposed by changes in IL that occur due to
>> transformations in the optimizer pipeline.
>>
>> Given the number of runtime triggers vs warnings, we know there's many
>> instances of passing 0 to the allocators that we're not diagnosing. I
>> can easily see differences in the early IL (such as those due to
>> BRANCH_COST differing for targets) exposing/hiding cases where 0 flows
>> into the allocator argument.  Similarly for changes in inlining
>> decisions, jump threading, etc for profiled bootstraps.  I'd like to
>> avoid playing wack-a-mole right now.
>>
>> So I'm being a bit more conservative here.  Maybe it'd be appropriate in
>> Wextra since that's not enabled by default for GCC builds.
>>
>
> actually it is enabled, by -W -Wall ...
>


Don't the gcc docs say that -Wextra is the preferred name for -W? Why
is gcc still using the deprecated name for the flag when building
itself? Seems like it'd help to avoid this confusion if the flags read
-Wextra instead of -W...

Re: [PATCH] avoid calling alloca(0)

[Bernd Edlinger]

On 12/01/16 19:39, Jeff Law wrote:
> On 11/30/2016 09:09 PM, Martin Sebor wrote:
>>> What I think this tells us is that we're not at a place where we're
>>> clean.  But we can incrementally get there.  The warning is only
>>> catching a fairly small subset of the cases AFAICT.  That's not unusual
>>> and analyzing why it didn't trigger on those cases might be useful as
>>> well.
>>
>> The warning has no smarts.  It relies on constant propagation and
>> won't find a call unless it sees it's being made with a constant
>> zero.  Looking at the top two on the list the calls are in extern
>> functions not called from the same source file, so it probably just
>> doesn't see that the functions are being called from another file
>> with a zero.  Building GCC with LTO might perhaps help.
> Right.  This is consistent with the limitations of other similar
> warnings such as null pointer dereferences.
>
>>
>>> So where does this leave us for gcc-7?  I'm wondering if we drop the
>>> warning in, but not enable it by default anywhere.  We fix the cases we
>>> can (such as reg-stack,c tree-ssa-threadedge.c, maybe others) before
>>> stage3 closes, and shoot for the rest in gcc-8, including improvign the
>>> warning (if there's something we can clearly improve), and enabling the
>>> warning in -Wall or -Wextra.
>>
>> I'm fine with deferring the GCC fixes and working on the cleanup
>> over time but I don't think that needs to gate enabling the option
>> with -Wextra.  The warnings can be suppressed or prevented from
>> causing errors during a GCC build either via a command line option
>> or by pragma in the code.  AFAICT, from the other warnings I see
>> go by, this is what has been done for -Wno-implicit-fallthrough
>> while those warnings are being cleaned up.  Why not take the same
>> approach here?
> The difference vs implicit fallthrough is that new instances of implicit
> fallthrus aren't likely to be exposed by changes in IL that occur due to
> transformations in the optimizer pipeline.
>
> Given the number of runtime triggers vs warnings, we know there's many
> instances of passing 0 to the allocators that we're not diagnosing. I
> can easily see differences in the early IL (such as those due to
> BRANCH_COST differing for targets) exposing/hiding cases where 0 flows
> into the allocator argument.  Similarly for changes in inlining
> decisions, jump threading, etc for profiled bootstraps.  I'd like to
> avoid playing wack-a-mole right now.
>
> So I'm being a bit more conservative here.  Maybe it'd be appropriate in
> Wextra since that's not enabled by default for GCC builds.
>

actually it is enabled, by -W -Wall ...

>
>>
>> As much as I would like to improve the warning itself I'm also not
>> sure I see much of an opportunity for it.  It's not prone to high
>> rates of false positives (hardly any in fact) and the cases it
>> misses are those where it simply doesn't see the argument value
>> because it's not been made available by constant propagation.
> There's always ways :-)   For example, I wouldn't be at all surprised if
> you found PHIs that feed the allocation where one or more of the PHI
> arguments are 0.
>
>
>>
>> That said, I consider the -Walloc-size-larger-than warning to be
>> the more important part of the patch by far.  I'd hate a lack of
>> consensus on how to deal with GCC's handful of instances of
>> alloca(0) to stall the rest of the patch.
> Agreed on not wanting alloca(0) handling to stall the rest of the patch.

I'd say negative arguments on alloca should always diagnosed,
as that does increment the stack in reverse direction.
And that is always very dangerous.

I think the default of -Walloc-size-larger-than should be changed as
Martin suggested to SIZE_T_MAX/2, I would make that the default.
And keep alloca and malloc size limits separate warnings.


Bernd.

Re: [PATCH] avoid calling alloca(0)

[Jeff Law]

On 11/30/2016 09:09 PM, Martin Sebor wrote:

What I think this tells us is that we're not at a place where we're
clean.  But we can incrementally get there.  The warning is only
catching a fairly small subset of the cases AFAICT.  That's not unusual
and analyzing why it didn't trigger on those cases might be useful as
well.


The warning has no smarts.  It relies on constant propagation and
won't find a call unless it sees it's being made with a constant
zero.  Looking at the top two on the list the calls are in extern
functions not called from the same source file, so it probably just
doesn't see that the functions are being called from another file
with a zero.  Building GCC with LTO might perhaps help.
Right.  This is consistent with the limitations of other similar 
warnings such as null pointer dereferences.





So where does this leave us for gcc-7?  I'm wondering if we drop the
warning in, but not enable it by default anywhere.  We fix the cases we
can (such as reg-stack,c tree-ssa-threadedge.c, maybe others) before
stage3 closes, and shoot for the rest in gcc-8, including improvign the
warning (if there's something we can clearly improve), and enabling the
warning in -Wall or -Wextra.


I'm fine with deferring the GCC fixes and working on the cleanup
over time but I don't think that needs to gate enabling the option
with -Wextra.  The warnings can be suppressed or prevented from
causing errors during a GCC build either via a command line option
or by pragma in the code.  AFAICT, from the other warnings I see
go by, this is what has been done for -Wno-implicit-fallthrough
while those warnings are being cleaned up.  Why not take the same
approach here?
The difference vs implicit fallthrough is that new instances of implicit 
fallthrus aren't likely to be exposed by changes in IL that occur due to 
transformations in the optimizer pipeline.


Given the number of runtime triggers vs warnings, we know there's many 
instances of passing 0 to the allocators that we're not diagnosing. I 
can easily see differences in the early IL (such as those due to 
BRANCH_COST differing for targets) exposing/hiding cases where 0 flows 
into the allocator argument.  Similarly for changes in inlining 
decisions, jump threading, etc for profiled bootstraps.  I'd like to 
avoid playing wack-a-mole right now.


So I'm being a bit more conservative here.  Maybe it'd be appropriate in 
Wextra since that's not enabled by default for GCC builds.





As much as I would like to improve the warning itself I'm also not
sure I see much of an opportunity for it.  It's not prone to high
rates of false positives (hardly any in fact) and the cases it
misses are those where it simply doesn't see the argument value
because it's not been made available by constant propagation.
There's always ways :-)   For example, I wouldn't be at all surprised if 
you found PHIs that feed the allocation where one or more of the PHI 
arguments are 0.





That said, I consider the -Walloc-size-larger-than warning to be
the more important part of the patch by far.  I'd hate a lack of
consensus on how to deal with GCC's handful of instances of
alloca(0) to stall the rest of the patch.

Agreed on not wanting alloca(0) handling to stall the rest of the patch.

Jeff

Re: [PATCH] avoid calling alloca(0)

[Martin Sebor]

On 11/30/2016 09:09 PM, Martin Sebor wrote:

What I think this tells us is that we're not at a place where we're
clean.  But we can incrementally get there.  The warning is only
catching a fairly small subset of the cases AFAICT.  That's not unusual
and analyzing why it didn't trigger on those cases might be useful as
well.


The warning has no smarts.  It relies on constant propagation and
won't find a call unless it sees it's being made with a constant
zero.  Looking at the top two on the list the calls are in extern
functions not called from the same source file, so it probably just
doesn't see that the functions are being called from another file
with a zero.  Building GCC with LTO might perhaps help.


I should also add that for GCC abd provided the main concern is
non-unique pointers, the warning find just the right subset of
calls,  (other concerns are portability to non-GCC compilers or
to library implementations).

GCC makes sure the size is a multiple of stack alignment. When
the argument is constant and a multiple of stack size GCC does
nothing, and so when the size is zero it just returns the top
of stack, resulting in non-unique pointers.  When it's not
constant, GCC emits code to round up the size to a multiple
of stack alignment, which makes each pointer unique.




So where does this leave us for gcc-7?  I'm wondering if we drop the
warning in, but not enable it by default anywhere.  We fix the cases we
can (such as reg-stack,c tree-ssa-threadedge.c, maybe others) before
stage3 closes, and shoot for the rest in gcc-8, including improvign the
warning (if there's something we can clearly improve), and enabling the
warning in -Wall or -Wextra.


I'm fine with deferring the GCC fixes and working on the cleanup
over time but I don't think that needs to gate enabling the option
with -Wextra.  The warnings can be suppressed or prevented from
causing errors during a GCC build either via a command line option
or by pragma in the code.  AFAICT, from the other warnings I see
go by, this is what has been done for -Wno-implicit-fallthrough
while those warnings are being cleaned up.  Why not take the same
approach here?

As much as I would like to improve the warning itself I'm also not
sure I see much of an opportunity for it.  It's not prone to high
rates of false positives (hardly any in fact) and the cases it
misses are those where it simply doesn't see the argument value
because it's not been made available by constant propagation.

That said, I consider the -Walloc-size-larger-than warning to be
the more important part of the patch by far.  I'd hate a lack of
consensus on how to deal with GCC's handful of instances of
alloca(0) to stall the rest of the patch.

Thanks
Martin

Re: [PATCH] avoid calling alloca(0)

[Martin Sebor]

What I think this tells us is that we're not at a place where we're
clean.  But we can incrementally get there.  The warning is only
catching a fairly small subset of the cases AFAICT.  That's not unusual
and analyzing why it didn't trigger on those cases might be useful as well.


The warning has no smarts.  It relies on constant propagation and
won't find a call unless it sees it's being made with a constant
zero.  Looking at the top two on the list the calls are in extern
functions not called from the same source file, so it probably just
doesn't see that the functions are being called from another file
with a zero.  Building GCC with LTO might perhaps help.


So where does this leave us for gcc-7?  I'm wondering if we drop the
warning in, but not enable it by default anywhere.  We fix the cases we
can (such as reg-stack,c tree-ssa-threadedge.c, maybe others) before
stage3 closes, and shoot for the rest in gcc-8, including improvign the
warning (if there's something we can clearly improve), and enabling the
warning in -Wall or -Wextra.


I'm fine with deferring the GCC fixes and working on the cleanup
over time but I don't think that needs to gate enabling the option
with -Wextra.  The warnings can be suppressed or prevented from
causing errors during a GCC build either via a command line option
or by pragma in the code.  AFAICT, from the other warnings I see
go by, this is what has been done for -Wno-implicit-fallthrough
while those warnings are being cleaned up.  Why not take the same
approach here?

As much as I would like to improve the warning itself I'm also not
sure I see much of an opportunity for it.  It's not prone to high
rates of false positives (hardly any in fact) and the cases it
misses are those where it simply doesn't see the argument value
because it's not been made available by constant propagation.

That said, I consider the -Walloc-size-larger-than warning to be
the more important part of the patch by far.  I'd hate a lack of
consensus on how to deal with GCC's handful of instances of
alloca(0) to stall the rest of the patch.

Thanks
Martin

Re: [PATCH] avoid calling alloca(0)

[Jeff Law]

On 11/26/2016 05:52 PM, Martin Sebor wrote:

On 11/25/2016 12:51 PM, Jeff Law wrote:

On 11/23/2016 06:15 PM, Martin Sebor wrote:


gcc_assert works only in some instances (e.g., in c-ada-spec.c:191)
but not in others because some actually do make the alloca(0) call
at runtime: at a minimum, lto.c:3285, reg-stack.c:2008, and
tree-ssa-threadedge.c:344 assert during bootstrap.

You might have the wrong line number of reg-stack.c and lto.  You've
pointed to the start of subst_asm_stack_regs and lto_main respectively.
It'd probably be better if you posted the line with a bit of context.


I must have copied the wrong line numbers or had stale sources
in my tree.  Sorry about that.  In lto.c, there are two calls
to XALLOCAVEC.  I believe the first one is the one where the
alloca(0) call takes place:

  1580
  1581  tree *map = XALLOCAVEC (tree, 2 * len);
  1582  for (tree_scc *pscc = *slot; pscc; pscc = pscc->next)
--
  1610{
  1611  tree *map2 = XALLOCAVEC (tree, 2 * len);
  1612  for (unsigned i = 0; i < len; ++i)
I'm not at all familiar with this code, but something looks real fishy 
here.  Essentially if pscc->entry_len is >= 1 and len == 0, then we'll 
read map[0] and map[1] which were never allocated (see compare_tree_sccs 
and compare_tree_sccs_1).


It may be the case that  pscc->entry_len and len are related in such a 
way that never happens, but I can't easily prove it.  I'd really like 
Richi to chime in on how this stuff is supposed to work.





In reg-stack.c it's these three:

  2052
  2053  note_reg = XALLOCAVEC (rtx, i);
  2054  note_loc = XALLOCAVEC (rtx *, i);
  2055  note_kind = XALLOCAVEC (enum reg_note, i);
  2056
So for reg-stack.c I think we move the n_notes initialization before the 
XALLOCAVEC, then wrap the XALLOCAVEC calls and the subsequent loop over 
the notes inside an if (i > 0) conditional.


Damn you for making me look at reg-stack.c.  It's been years and 
hopefully it'll be years before I have to do it again :-)



n_notes = 0;
if (i > 0)
  {
note_reg =
note_loc =
note_kind =

for (note = REG_NOTES (insn); ...)
  {
 ...
  }
  }


I'm pretty sure I can twiddle the tree-ssa-threadedge code to avoid the 
problem in there.




To find all such calls I modified GCC to emit an inform call for
every XALLOCAVEC invocation with a zero argument, configured the
patched GCC on x86_64 with all languages (including lto),
bootstrapped it, ran the full test suite, and extracted the set
of unique notes from the logs.  Attached in the .log file is
the output along with counts of each.  Curiously, neither of
the two above shows up, even though adding asserts for them
broke bootstrap.  I haven't investigated why.
Thanks.  That's interesting data -- every one of those should be deeply 
investigated.  I suspect we'd probably trip even more if we did a test 
with config-list.mk and perhaps even more if we took those cross 
compilers and built the target libraries.


What I think this tells us is that we're not at a place where we're 
clean.  But we can incrementally get there.  The warning is only 
catching a fairly small subset of the cases AFAICT.  That's not unusual 
and analyzing why it didn't trigger on those cases might be useful as well.


So where does this leave us for gcc-7?  I'm wondering if we drop the 
warning in, but not enable it by default anywhere.  We fix the cases we 
can (such as reg-stack,c tree-ssa-threadedge.c, maybe others) before 
stage3 closes, and shoot for the rest in gcc-8, including improvign the 
warning (if there's something we can clearly improve), and enabling the 
warning in -Wall or -Wextra.



Jeff

Re: [PATCH] avoid calling alloca(0)

[Martin Sebor]

On 11/25/2016 12:51 PM, Jeff Law wrote:

On 11/23/2016 06:15 PM, Martin Sebor wrote:


gcc_assert works only in some instances (e.g., in c-ada-spec.c:191)
but not in others because some actually do make the alloca(0) call
at runtime: at a minimum, lto.c:3285, reg-stack.c:2008, and
tree-ssa-threadedge.c:344 assert during bootstrap.

You might have the wrong line number of reg-stack.c and lto.  You've
pointed to the start of subst_asm_stack_regs and lto_main respectively.
It'd probably be better if you posted the line with a bit of context.


I must have copied the wrong line numbers or had stale sources
in my tree.  Sorry about that.  In lto.c, there are two calls
to XALLOCAVEC.  I believe the first one is the one where the
alloca(0) call takes place:

  1580  
  1581tree *map = XALLOCAVEC (tree, 2 * len);
  1582for (tree_scc *pscc = *slot; pscc; pscc = pscc->next)
--
  1610  {
  1611tree *map2 = XALLOCAVEC (tree, 2 * len);
  1612for (unsigned i = 0; i < len; ++i)

In reg-stack.c it's these three:

  2052  
  2053note_reg = XALLOCAVEC (rtx, i);
  2054note_loc = XALLOCAVEC (rtx *, i);
  2055note_kind = XALLOCAVEC (enum reg_note, i);
  2056  

To find all such calls I modified GCC to emit an inform call for
every XALLOCAVEC invocation with a zero argument, configured the
patched GCC on x86_64 with all languages (including lto),
bootstrapped it, ran the full test suite, and extracted the set
of unique notes from the logs.  Attached in the .log file is
the output along with counts of each.  Curiously, neither of
the two above shows up, even though adding asserts for them
broke bootstrap.  I haven't investigated why.

Martin

PS The patch I used to get the output is in the attached .diff
file.
gcc/ada/gcc-interface/utils.c:5623: void def_fn_type(builtin_type, builtin_type, bool, int, ...): alloca called with a zero argument
gcc/calls.c:3260: rtx_def* expand_call(tree, rtx, int): alloca called with a zero argument
gcc/c-family/c-common.c:3914: void def_fn_type(builtin_type, builtin_type, bool, int, ...): alloca called with a zero argument
gcc/cp/call.c:3141: z_candidate* add_template_candidate_real(z_candidate**, tree, tree, tree, tree, const vec*, tree, tree, tree, int, tree, unification_kind_t, tsubst_flags_t): alloca called with a zero argument
gcc/cp/pt.c:11362: tree_node* tsubst_template_args(tree, tree, tsubst_flags_t, tree): alloca called with a zero argument
gcc/cp/semantics.c:1444: tree_node* finish_asm_stmt(int, tree, tree, tree, tree, tree): alloca called with a zero argument
gcc/final.c:2632: rtx_insn* final_scan_insn(rtx_insn*, FILE*, int, int, int*): alloca called with a zero argument
gcc/gimple-fold.c:4346: bool fold_stmt_1(gimple_stmt_iterator*, bool, tree_node* (*)(tree)): alloca called with a zero argument
gcc/gimple-fold.c:5852: tree_node* gimple_fold_stmt_to_constant_1(gimple*, tree_node* (*)(tree), tree_node* (*)(tree)): alloca called with a zero argument
gcc/gimple-walk.c:844: bool walk_stmt_load_store_addr_ops(gimple*, void*, walk_stmt_load_store_addr_fn, walk_stmt_load_store_addr_fn, walk_stmt_load_store_addr_fn): alloca called with a zero argument
gcc/tree.c:11260: tree_node* build_call_expr_loc(location_t, tree, int, ...): alloca called with a zero argument
gcc/tree.c:11277: tree_node* build_call_expr(tree, int, ...): alloca called with a zero argument
gcc/tree.c:11312: tree_node* build_call_expr_internal_loc(location_t, internal_fn, tree, int, ...): alloca called with a zero argument
gcc/tree-ssa-structalias.c:4930: void find_func_aliases(function*, gimple*): alloca called with a zero argument
gcc/tree-ssa-threadedge.c:343: gimple* record_temporary_equivalences_from_stmts_at_dest(edge, const_and_copies*, avail_exprs_stack*, tree_node* (*)(gimple*, gimple*, avail_exprs_stack*)): alloca called with a zero argument


   CALLS   LOCATION
  --   
  226872   /src/gcc/78284/gcc/c-family/c-common.c:3914
  117141   /src/gcc/78284/gcc/calls.c:3260
  183040   /src/gcc/78284/gcc/cp/call.c:3141
   80400   /src/gcc/78284/gcc/ada/gcc-interface/utils.c:5623
   17671   /src/gcc/78284/gcc/gimple-fold.c:4346
   65348   /src/gcc/78284/gcc/gimple-fold.c:5852
   63468   /src/gcc/78284/gcc/tree-ssa-threadedge.c:343
   32886   /src/gcc/78284/gcc/gimple-walk.c:844
6578   /src/gcc/78284/gcc/tree-ssa-structalias.c:4930
4046   /src/gcc/78284/gcc/cp/pt.c:11362
4866   /src/gcc/78284/gcc/cp/semantics.c:1444
1484   /src/gcc/78284/gcc/final.c:2632
  80   /src/gcc/78284/gcc/tree.c:11312
  26   /src/gcc/78284/gcc/tree.c:11260
   4   /src/gcc/78284/gcc/tree.c:11277
diff --git a/gcc/tree-core.h b/gcc/tree-core.h
index 3e3f31e..24c8c32 100644
--- a/gcc/tree-core.h
+++ b/gcc/tree-core.h
@@ -22,6 +22,12 @@ along with GCC; see the file COPYING3.  If not see
 
 #include "symtab.h"
 
+extern void inform (location_t, const char *, ...);
+
+#undef WARN_ALLOCA_ZERO
+#define WARN_ALLOCA_ZERO() \
+  

Re: [PATCH] avoid calling alloca(0)

[Jeff Law]

On 11/23/2016 06:15 PM, Martin Sebor wrote:


gcc_assert works only in some instances (e.g., in c-ada-spec.c:191)
but not in others because some actually do make the alloca(0) call
at runtime: at a minimum, lto.c:3285, reg-stack.c:2008, and
tree-ssa-threadedge.c:344 assert during bootstrap.
You might have the wrong line number of reg-stack.c and lto.  You've 
pointed to the start of subst_asm_stack_regs and lto_main respectively. 
It'd probably be better if you posted the line with a bit of context.


I can trivially fix the threadedge issue.

Jeff

Re: [PATCH] avoid calling alloca(0)

[Jeff Law]

On 11/24/2016 12:42 AM, Jakub Jelinek wrote:

After reviewing a few more of the XALLOCAVEC calls in the affected
files I suspect that those to alloca(0) pointed out by the warning
may be just a subset that GCC happens to see thanks to constant
propagation.  If that's so then changing this subset to
alloca(N + !N) or some such is probably not the right approach
because it will miss all the other calls where GCC doesn't see that
N is zero.  I think the most robust solution is to do as Bernd
suggests: change XALLOCAVEC as shown above.  That will let us
prevent any and all unsafe assumptions about the result of
alloca(0) being either non-null or distinct.  The other approach
would be to change XALLOCAVEC to add 1 to the byte count.  That
would be in line with what XMALLOC does.


I still fail to see why you want to change anything at least for
hosts where you know XALLOCAVEC is __builtin_alloca.
Show me one place which assumes the result of alloca (0) in gcc sources is
distinct, or non-NULL.  For 0 elements the pointer simply isn't used.
And whether for the malloc based alloca it GCs or not really doesn't matter
for us.
I think for host/build, we ought to assume that alloca is 
__builtin_alloca.  I think we stopped supporting the 
alloca-on-top-of-malloc host/build systems long ago.


But I still think we ought to be "clean" in regard to zero sized 
allocations.  It sounds like an assert may not be sufficient, so we need 
to look at another approach.


Jeff

Re: [PATCH] avoid calling alloca(0)

[Jeff Law]

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

Re: [PATCH] avoid calling alloca(0)

[Jakub Jelinek]

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

Re: [PATCH] avoid calling alloca(0)

[Jakub Jelinek]

On Wed, Nov 23, 2016 at 06:15:11PM -0700, Martin Sebor wrote:
> >Can't we just
> >gcc_assert (x != 0) before the problematical calls?  That avoids
> >unnecessary over-allocation and gets us a clean ICE if we were to try
> >and call alloca with a problematical value.
> 
> gcc_assert works only in some instances (e.g., in c-ada-spec.c:191)
> but not in others because some actually do make the alloca(0) call
> at runtime: at a minimum, lto.c:3285, reg-stack.c:2008, and
> tree-ssa-threadedge.c:344 assert during bootstrap.
> 
> After reviewing a few more of the XALLOCAVEC calls in the affected
> files I suspect that those to alloca(0) pointed out by the warning
> may be just a subset that GCC happens to see thanks to constant
> propagation.  If that's so then changing this subset to
> alloca(N + !N) or some such is probably not the right approach
> because it will miss all the other calls where GCC doesn't see that
> N is zero.  I think the most robust solution is to do as Bernd
> suggests: change XALLOCAVEC as shown above.  That will let us
> prevent any and all unsafe assumptions about the result of
> alloca(0) being either non-null or distinct.  The other approach
> would be to change XALLOCAVEC to add 1 to the byte count.  That
> would be in line with what XMALLOC does.

I still fail to see why you want to change anything at least for
hosts where you know XALLOCAVEC is __builtin_alloca.
Show me one place which assumes the result of alloca (0) in gcc sources is
distinct, or non-NULL.  For 0 elements the pointer simply isn't used.
And whether for the malloc based alloca it GCs or not really doesn't matter
for us.

Jakub

Re: [PATCH] avoid calling alloca(0)

[Martin Sebor]

On 11/23/2016 01:57 PM, Jeff Law wrote:

On 11/20/2016 04:06 PM, Martin Sebor wrote:

On 11/20/2016 01:03 AM, Bernd Edlinger wrote:

On 11/20/16 00:43, Martin Sebor wrote:

As best I can tell the result isn't actually used (the code that
uses the result gets branched over).  GCC just doesn't see it.
I verified this by changing the XALLOCAVEC macro to

  #define XALLOCAVEC(T, N)  (N ? alloca (sizeof (T) * N) : 0)

and bootstrapping and running the regression test suite with
no apparent regressions.



I would like this solution with braces around N better than
allocating one element when actually zero were requested.

The disadvantage of N+1 or N+!N is that it will hide true
programming errors from the sanitizer.


I agree.  Let me post an updated patch with the above fix and
leave it to others to choose which approach to go with.

Just so I'm clear, this part of the discussions is talking about
mitigation strategies within GCC itself, right?


That's right.


Can't we just
gcc_assert (x != 0) before the problematical calls?  That avoids
unnecessary over-allocation and gets us a clean ICE if we were to try
and call alloca with a problematical value.


gcc_assert works only in some instances (e.g., in c-ada-spec.c:191)
but not in others because some actually do make the alloca(0) call
at runtime: at a minimum, lto.c:3285, reg-stack.c:2008, and
tree-ssa-threadedge.c:344 assert during bootstrap.

After reviewing a few more of the XALLOCAVEC calls in the affected
files I suspect that those to alloca(0) pointed out by the warning
may be just a subset that GCC happens to see thanks to constant
propagation.  If that's so then changing this subset to
alloca(N + !N) or some such is probably not the right approach
because it will miss all the other calls where GCC doesn't see that
N is zero.  I think the most robust solution is to do as Bernd
suggests: change XALLOCAVEC as shown above.  That will let us
prevent any and all unsafe assumptions about the result of
alloca(0) being either non-null or distinct.  The other approach
would be to change XALLOCAVEC to add 1 to the byte count.  That
would be in line with what XMALLOC does.


I'm not sure we need to break things up.  I haven't seen a good argument
for that yet.

Is there an updated patch to post?  Or has it already been posted?


Given the above I suggest going with one of the attached two patches.

Martin


include/ChangeLog:

	* libiberty.h (XALLOCAVEC): Return null for zero-size allocations.

Index: include/libiberty.h
===
--- include/libiberty.h	(revision 242768)
+++ include/libiberty.h	(working copy)
@@ -353,7 +353,8 @@ extern unsigned int xcrc32 (const unsigned char *,
 
 /* Array allocators.  */
 
-#define XALLOCAVEC(T, N)	((T *) alloca (sizeof (T) * (N)))
+#define XALLOCAVEC(T, N) \
+  (T *) (((N) != 0) ? alloca (sizeof (T) * (N)) : 0)
 #define XNEWVEC(T, N)		((T *) xmalloc (sizeof (T) * (N)))
 #define XCNEWVEC(T, N)		((T *) xcalloc ((N), sizeof (T)))
 #define XDUPVEC(T, P, N)	((T *) xmemdup ((P), sizeof (T) * (N), sizeof (T) * (N)))
include/ChangeLog:

	* libiberty.h (XALLOCAVEC): Avoid calling alloca with a zero argument.

Index: include/libiberty.h
===
--- include/libiberty.h	(revision 242768)
+++ include/libiberty.h	(working copy)
@@ -353,7 +353,7 @@ extern unsigned int xcrc32 (const unsigned char *,
 
 /* Array allocators.  */
 
-#define XALLOCAVEC(T, N)	((T *) alloca (sizeof (T) * (N)))
+#define XALLOCAVEC(T, N)	((T *) alloca (sizeof (T) * (N) + 1))
 #define XNEWVEC(T, N)		((T *) xmalloc (sizeof (T) * (N)))
 #define XCNEWVEC(T, N)		((T *) xcalloc ((N), sizeof (T)))
 #define XDUPVEC(T, P, N)	((T *) xmemdup ((P), sizeof (T) * (N), sizeof (T) * (N)))

Re: [PATCH] avoid calling alloca(0)

[Jeff Law]

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

Re: [PATCH] avoid calling alloca(0)

[Jeff Law]

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

Re: [PATCH] avoid calling alloca(0)

[Jeff Law]

On 11/20/2016 04:06 PM, Martin Sebor wrote:

On 11/20/2016 01:03 AM, Bernd Edlinger wrote:

On 11/20/16 00:43, Martin Sebor wrote:

As best I can tell the result isn't actually used (the code that
uses the result gets branched over).  GCC just doesn't see it.
I verified this by changing the XALLOCAVEC macro to

  #define XALLOCAVEC(T, N)  (N ? alloca (sizeof (T) * N) : 0)

and bootstrapping and running the regression test suite with
no apparent regressions.



I would like this solution with braces around N better than
allocating one element when actually zero were requested.

The disadvantage of N+1 or N+!N is that it will hide true
programming errors from the sanitizer.


I agree.  Let me post an updated patch with the above fix and
leave it to others to choose which approach to go with.
Just so I'm clear, this part of the discussions is talking about 
mitigation strategies within GCC itself, right?   Can't we just
gcc_assert (x != 0) before the problematical calls?  That avoids 
unnecessary over-allocation and gets us a clean ICE if we were to try 
and call alloca with a problematical value.



ed in the first patch.  I'll post an updated patch soon.




I am a bit worried about that suggestion in libiberty/alloca.c:
"It is a good idea to use alloca(0) in
  your main control loop, etc. to force garbage collection."

Because:

#include 
#include 
int main()
{
   void* p;
   int i;
   for (i=0 ; i<100 ; i++)
   {
 p = alloca(0);
 printf("%p\n", p);
   }
}

... is fine, and allocates no memory.
But if I change that to alloca(1) the stack will blow up.


The suggestion to call alloca(0) in the main loop is to trigger
garbage collection when alloca is implemented using malloc (as
is apparently the case in some embedded environments).  In that
case, alloca is not a built-in but a library function (i.e.,
-fno-builtin-alloca must be set) and the warning doesn't trigger.

Correct.



This call to alloca(0) would also likely be made without using
the return value.  When __builtin_alloca is called without using
the return value, the call is eliminated and the warning doesn't
trigger either.

Also correct.


There is a -Walloca-larger-than that Aldy added earlier this
year.  The option also diagnoses alloca(0), and it also warns
on calls to alloca in loops, but it's disabled by default.
I feel fairly strongly that all zero-length allocations
should be diagnosed at least with -Wextra but if that's what
it takes to move forward I'm willing to break things up this
way.  If that's preferred then I would also expect to enable
-Walloca-larger-than=SIZE_MAX/2 (or smaller) by default, in
addition to having -Walloca-zero in -Wextra, analogously to
the corresponding warnings for the heap allocation functions.
(Ditto for -Wvla-larger-than=SIZE_MAX/2 and -Wvla-zero which
should probably also be added for consistency.)
I'm not sure we need to break things up.  I haven't seen a good argument 
for that yet.


Is there an updated patch to post?  Or has it already been posted?

jeff

Re: [PATCH] avoid calling alloca(0)

[Martin Sebor]

On 11/20/2016 01:03 AM, Bernd Edlinger wrote:

On 11/20/16 00:43, Martin Sebor wrote:

As best I can tell the result isn't actually used (the code that
uses the result gets branched over).  GCC just doesn't see it.
I verified this by changing the XALLOCAVEC macro to

  #define XALLOCAVEC(T, N)  (N ? alloca (sizeof (T) * N) : 0)

and bootstrapping and running the regression test suite with
no apparent regressions.



I would like this solution with braces around N better than
allocating one element when actually zero were requested.

The disadvantage of N+1 or N+!N is that it will hide true
programming errors from the sanitizer.


I agree.  Let me post an updated patch with the above fix and
leave it to others to choose which approach to go with.




you should also include glibc and busybox, to be sure.


Thanks for the suggestion.  I've done that and found no instances
of any of these warnings in either Busybox 1.25.1 or Glibc trunk.



Good.  I see many alloca calls all over, but mostly in the runtime
libraries, that don't use -Werror.
Have you done a bootstrap with all languages?
Were there warnings in the target libraries (note they don't use
-Werror, so you have to grep)?


Yes, I bootstrapped all languages.  When testing my latest patch
yesterday I found a couple more places in GCC (one in Ada and
another in the  middle end I think) that trigger the warning that
I had missed in the first patch.  I'll post an updated patch soon.



I am a bit worried about that suggestion in libiberty/alloca.c:
"It is a good idea to use alloca(0) in
  your main control loop, etc. to force garbage collection."

Because:

#include 
#include 
int main()
{
   void* p;
   int i;
   for (i=0 ; i<100 ; i++)
   {
 p = alloca(0);
 printf("%p\n", p);
   }
}

... is fine, and allocates no memory.
But if I change that to alloca(1) the stack will blow up.


The suggestion to call alloca(0) in the main loop is to trigger
garbage collection when alloca is implemented using malloc (as
is apparently the case in some embedded environments).  In that
case, alloca is not a built-in but a library function (i.e.,
-fno-builtin-alloca must be set) and the warning doesn't trigger.

This call to alloca(0) would also likely be made without using
the return value.  When __builtin_alloca is called without using
the return value, the call is eliminated and the warning doesn't
trigger either.



Maybe it would be better to have a different warning option
for malloc/realloc with zero and for alloca with zero?


There is a -Walloca-larger-than that Aldy added earlier this
year.  The option also diagnoses alloca(0), and it also warns
on calls to alloca in loops, but it's disabled by default.
I feel fairly strongly that all zero-length allocations
should be diagnosed at least with -Wextra but if that's what
it takes to move forward I'm willing to break things up this
way.  If that's preferred then I would also expect to enable
-Walloca-larger-than=SIZE_MAX/2 (or smaller) by default, in
addition to having -Walloca-zero in -Wextra, analogously to
the corresponding warnings for the heap allocation functions.
(Ditto for -Wvla-larger-than=SIZE_MAX/2 and -Wvla-zero which
should probably also be added for consistency.)

Martin

Re: [PATCH] avoid calling alloca(0)

[Martin Sebor]

On 11/19/2016 11:18 PM, Jakub Jelinek wrote:

On Sat, Nov 19, 2016 at 04:43:29PM -0700, Martin Sebor wrote:

Thanks for calling out the realloc(0, p) case!  Realloc(0, p) is


??  The DR you refer to deprecates realloc(p, 0), not realloc(0, p).


We are discussing zero size allocation so I was obviously referring
to realloc(p, 0).  Bernd had the arguments in the right order in
his message and I transposed them by mistake in my reply.

Martin

Re: [PATCH] avoid calling alloca(0)

[Bernd Edlinger]

On 11/20/16 00:43, Martin Sebor wrote:
> As best I can tell the result isn't actually used (the code that
> uses the result gets branched over).  GCC just doesn't see it.
> I verified this by changing the XALLOCAVEC macro to
>
>   #define XALLOCAVEC(T, N)  (N ? alloca (sizeof (T) * N) : 0)
>
> and bootstrapping and running the regression test suite with
> no apparent regressions.
>

I would like this solution with braces around N better than
allocating one element when actually zero were requested.

The disadvantage of N+1 or N+!N is that it will hide true
programming errors from the sanitizer.

>> you should also include glibc and busybox, to be sure.
>
> Thanks for the suggestion.  I've done that and found no instances
> of any of these warnings in either Busybox 1.25.1 or Glibc trunk.
>

Good.  I see many alloca calls all over, but mostly in the runtime
libraries, that don't use -Werror.
Have you done a bootstrap with all languages?
Were there warnings in the target libraries (note they don't use
-Werror, so you have to grep)?


I am a bit worried about that suggestion in libiberty/alloca.c:
"It is a good idea to use alloca(0) in
  your main control loop, etc. to force garbage collection."

Because:

#include 
#include 
int main()
{
   void* p;
   int i;
   for (i=0 ; i<100 ; i++)
   {
 p = alloca(0);
 printf("%p\n", p);
   }
}

... is fine, and allocates no memory.
But if I change that to alloca(1) the stack will blow up.

Maybe it would be better to have a different warning option
for malloc/realloc with zero and for alloca with zero?


Bernd.

Re: [PATCH] avoid calling alloca(0)

[Jakub Jelinek]

On Sat, Nov 19, 2016 at 04:43:29PM -0700, Martin Sebor wrote:
> Thanks for calling out the realloc(0, p) case!  Realloc(0, p) is

??  The DR you refer to deprecates realloc(p, 0), not realloc(0, p).
The latter is used much more widely, e.g. by not special casing
the first allocation.  So you use
  void *p = NULL;
  for (...)
{
  void *q = realloc (p, sz);
  if (q == NULL)
whatever;
  p = q;
  ...
}

Jakub

Re: [PATCH] avoid calling alloca(0)

[Martin Sebor]

So far I thought the warning is trying to make no differences between
malloc, realloc and alloca.

I would say that using realloc(x,0) is for sure always wrong.
Nobody will object against a warning for that.


Thanks for calling out the realloc(0, p) case!  Realloc(0, p) is
impossible to use portably and has been deprecated in C11 in
response to defect report 400:
http://www.open-std.org/jtc1/sc22/wg14/www/docs/summary.htm#dr_400
This change will be published in the 2017 technical corrigendum to
the C11 standard.


And yes, malloc(0) is unportable, but if the result is not used and
directly fed into free that should be no problem, but a warning would
be also helpful because it is unportable code.  But I hope that it is
not the same as with the false positive array bounds warnings where
the compiler first transforms the source code into something completely
different and then starts to warn about it.


I'm not aware of any such transformation or false positives due to
it but if you have more details or a test case I'll verify that it
doesn't trip it up.


Regarding alloca, there are probably three different forms.

First a function that is not a builtin but has the name "alloca"
like special_function_p understands it.  It has no attributes
unless the header mentions them.  Calling this function with
0 should not be warned about, right?


Right.  (I assume this means -fno-builtin-alloca or its equivalent
such as -ffreestanding; otherwise GCC mostly treats "alloca" the
same as "__builtin_alloca", with the exception of special_function_p).



Then a function that has the name "alloca" and matches the signature
of the builtin "alloca" function.


Right.  (This means that -fbuiltin-alloca is set (i.e., without
an explicit -fno-builtin-alloca, -fno-builtin, or -ffreestanding).



And last a function that has the name "__builtin_alloca".


Right.  (Either a direct call to it or as a result of macro
expansion like in Glibc .)



You can distinguish between these, and possibly only warn for
__builtin_alloca?  Note, that will depend on the way the glibc
header works.


Yes, it's possible to distinguish these cases.  The last patch
I posted doesn't warn on case (1) when -fno-builtin-alloca is
set because then the function code isn't BUILT_IN_ALLOCA.  It
does warn on case (2) because it only looks at the function
code.  I offered to make it not warn on case (2) to help avoid
changing calls to it from alloca(n) to alloca(n + !n) when n is
determined to be zero by constant propagation.


Everything would be more easy if the glibc header would not do
that, and just use the alloca and no macro.  Then it would be
more natural to warn about alloca and not about __builtin_alloca,
because that is always implemented in a sensible way.

But even if the __builtin_alloca is called with 0, what do we
do with the result?  I mean any use of the value would be wrong.
So why is there a warning, when the value is not used?


As best I can tell the result isn't actually used (the code that
uses the result gets branched over).  GCC just doesn't see it.
I verified this by changing the XALLOCAVEC macro to

  #define XALLOCAVEC(T, N)  (N ? alloca (sizeof (T) * N) : 0)

and bootstrapping and running the regression test suite with
no apparent regressions.


If you or others are concerned about the rate of false positives
of this warning please point me at a code base that might be a good
test bed to to try it on.  Besides GCC I've built Binutils and the
Linux kernel with just the 5 instances in GCC.



you should also include glibc and busybox, to be sure.


Thanks for the suggestion.  I've done that and found no instances
of any of these warnings in either Busybox 1.25.1 or Glibc trunk.

Martin

Re: [PATCH] avoid calling alloca(0)

[Bernd Edlinger]

On 11/19/16 00:52, Martin Sebor wrote:
> If you or others are concerned about the rate of false positives
> of this warning please point me at a code base that might be a good
> test bed to to try it on.  Besides GCC I've built Binutils and the
> Linux kernel with just the 5 instances in GCC.
>

you should also include glibc and busybox, to be sure.


Bernd.

Re: [PATCH] avoid calling alloca(0)

[Bernd Edlinger]

On 11/19/16 00:52, Martin Sebor wrote:
> On 11/18/2016 03:51 PM, Bernd Edlinger wrote:
>>  > 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
>>
>> No. I doubt that will work, unless you use -ffreestanding on the
>> command line.
>
> It works because "__builtin_alloca" is distinct from "alloca" and
> the warning code can simply compare the strings, just as you say
> you did below.  It's not ideal and I would prefer to avoid it but
> I offer it as another solution if the performance overhead of
> (n + 1) is thought to be too high.
>

So far I thought the warning is trying to make no differences between
malloc, realloc and alloca.

I would say that using realloc(x,0) is for sure always wrong.
Nobody will object against a warning for that.

And yes, malloc(0) is unportable, but if the result is not used and
directly fed into free that should be no problem, but a warning would
be also helpful because it is unportable code.  But I hope that it is
not the same as with the false positive array bounds warnings where
the compiler first transforms the source code into something completely
different and then starts to warn about it.

Regarding alloca, there are probably three different forms.

First a function that is not a builtin but has the name "alloca"
like special_function_p understands it.  It has no attributes
unless the header mentions them.  Calling this function with
0 should not be warned about, right?

Then a function that has the name "alloca" and matches the signature
of the builtin "alloca" function.

And last a function that has the name "__builtin_alloca".

You can distinguish between these, and possibly only warn for
__builtin_alloca?  Note, that will depend on the way the glibc
header works.

Everything would be more easy if the glibc header would not do
that, and just use the alloca and no macro.  Then it would be
more natural to warn about alloca and not about __builtin_alloca,
because that is always implemented in a sensible way.

But even if the __builtin_alloca is called with 0, what do we
do with the result?  I mean any use of the value would be wrong.
So why is there a warning, when the value is not used?



Bernd.

Re: [PATCH] avoid calling alloca(0)

[Martin Sebor]

On 11/18/2016 03:51 PM, Bernd Edlinger wrote:

 > 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

No. I doubt that will work, unless you use -ffreestanding on the
command line.


It works because "__builtin_alloca" is distinct from "alloca" and
the warning code can simply compare the strings, just as you say
you did below.  It's not ideal and I would prefer to avoid it but
I offer it as another solution if the performance overhead of
(n + 1) is thought to be too high.


Although the macro is not used in this case, even a declaraion
like

extern void *alloca (size_t __size);

goes thru the decl-anticipated path, that means it inherits
all the attributes from the builtin, unless the parameter don't
match, in that case you get a warning in C but no warning
in C++ (I am going to change the latter).

There is currently an attribute malloc but no explicit attribute
alloca, that's one of the reasons why special_function_p still
has to do a string compare of the function name aginst "alloca".

I think Jakub is right that we need a better way to fix the
warning for instance with a comment like the fall thru thing.


I don't believe this rare, corner case justifies special treatment.
There are many warnings in both -Wall and -Wextra that are orders
of magnitude noisier and that people find useful nonetheless.  They
don't need a special mechanism to suppress or fine tune beyond what
GCC already provides: an option and a pragma, and neither should
this one.  I certainly don't think that extending the complicated
(and controversial) machinery that was put in place to deal with
the exceedingly noisy fallthrough warning would be worth the effort
or even appropriate.

If you or others are concerned about the rate of false positives
of this warning please point me at a code base that might be a good
test bed to to try it on.  Besides GCC I've built Binutils and the
Linux kernel with just the 5 instances in GCC.

Martin

Re: [PATCH] avoid calling alloca(0)

[Bernd Edlinger]

 > 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

No. I doubt that will work, unless you use -ffreestanding on the
command line.

Although the macro is not used in this case, even a declaraion
like

extern void *alloca (size_t __size);

goes thru the decl-anticipated path, that means it inherits
all the attributes from the builtin, unless the parameter don't
match, in that case you get a warning in C but no warning
in C++ (I am going to change the latter).

There is currently an attribute malloc but no explicit attribute
alloca, that's one of the reasons why special_function_p still
has to do a string compare of the function name aginst "alloca".

I think Jakub is right that we need a better way to fix the
warning for instance with a comment like the fall thru thing.

Complicated code changes can also introduce new errors.


Bernd.

Re: [PATCH] avoid calling alloca(0)

[Martin Sebor]

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.

Re: [PATCH] avoid calling alloca(0)

[Martin Sebor]

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

Re: [PATCH] avoid calling alloca(0)

[Jeff Law]

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

Re: [PATCH] avoid calling alloca(0)

[Jeff Law]

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

Re: [PATCH] avoid calling alloca(0)

[Jakub Jelinek]

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

Re: [PATCH] avoid calling alloca(0)

[Martin Sebor]

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

Re: [PATCH] avoid calling alloca(0)

[Eric Gallager]

On 11/18/16, 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.
>


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 
#include 

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.

Re: [PATCH] avoid calling alloca(0)

[Jakub Jelinek]

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

Re: [PATCH] avoid calling alloca(0)

[Martin Sebor]

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

Re: [PATCH] avoid calling alloca(0)

[Jakub Jelinek]

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

Re: [PATCH] avoid calling alloca(0)

[Martin Sebor]

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

Re: [PATCH] avoid calling alloca(0)

[Martin Sebor]

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

Re: [PATCH] avoid calling alloca(0)

[Jeff Law]

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

Re: [PATCH] avoid calling alloca(0)

[Jakub Jelinek]

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

Re: [PATCH] avoid calling alloca(0)

[Jeff Law]

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

Re: [PATCH] avoid calling alloca(0)

[Jakub Jelinek]

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