Re: Avoid alloca(0) when temporarily propagating operands during threading
On Fri, Dec 02, 2016 at 11:13:29AM -0700, Jeff Law wrote: > >But -Wuninitialized also found tons of real-world bugs. Do we have a single > >real-world example where such a warning would actually be useful (as in, > >there would be an actual bug)? Otherwise we are adding workarounds for a > >warning that just forces people to add those workarounds, but doesn't > >improve code in the wild in any way. > Have you looked in depth at the lto.c issue it flagged? I can't prove that > one is safe. If you mean the tree *map = XALLOCAVEC (tree, 2 * len); call, then I strongly doubt it can actually ever be called with len == 0. There is tree_scc *scc = (tree_scc *) alloca (sizeof (tree_scc) + (len - 1) * sizeof (tree)); a few lines above this and len is unsigned int, therefore for len 0 this will try to (on 64-bit host) alloca (32 + 0x * 4UL), i.e. alloca (0x4001cUL) and I'm pretty sure on most hosts alloca of 16GB won't really work. Even if this succeeded for whatever reason, what problem do you see with map = alloca (0)? > And more generally, an under-sized allocation is a security risk. We should Sure. But I really don't see 0 as being special in any way. We do support zero sized arrays and IMNSHO we want to support 0 sized alloca, it is very much the same thing. We also do support VLAs with 0 size. Jakub
Re: Avoid alloca(0) when temporarily propagating operands during threading
On 12/02/2016 11:11 AM, Jakub Jelinek wrote: On Fri, Dec 02, 2016 at 11:02:33AM -0700, Jeff Law wrote: It won't cause any problems in this and probably most instances, but leaving the code in its prior state is simply wrong from a maintenance standpoint. I'd much rather have the code explicitly and safely handle the zero operands case so that if someone makes a change later they don't have to worry about whether or not they're accessing memory which was never allocated. Additionally, it removes a false positive from the warning, thus making less noise. It's not unlike the strictly unnecessary initializations we do to shut up -Wuninitialized. But -Wuninitialized also found tons of real-world bugs. Do we have a single real-world example where such a warning would actually be useful (as in, there would be an actual bug)? Otherwise we are adding workarounds for a warning that just forces people to add those workarounds, but doesn't improve code in the wild in any way. Have you looked in depth at the lto.c issue it flagged? I can't prove that one is safe. And more generally, an under-sized allocation is a security risk. We should continue to try and identify those and warn for them. jeff
Re: Avoid alloca(0) when temporarily propagating operands during threading
On Fri, Dec 02, 2016 at 11:02:33AM -0700, Jeff Law wrote: > It won't cause any problems in this and probably most instances, but leaving > the code in its prior state is simply wrong from a maintenance standpoint. > > I'd much rather have the code explicitly and safely handle the zero operands > case so that if someone makes a change later they don't have to worry about > whether or not they're accessing memory which was never allocated. > > Additionally, it removes a false positive from the warning, thus making less > noise. > > It's not unlike the strictly unnecessary initializations we do to shut up > -Wuninitialized. But -Wuninitialized also found tons of real-world bugs. Do we have a single real-world example where such a warning would actually be useful (as in, there would be an actual bug)? Otherwise we are adding workarounds for a warning that just forces people to add those workarounds, but doesn't improve code in the wild in any way. Jakub
Re: Avoid alloca(0) when temporarily propagating operands during threading
On 12/02/2016 10:58 AM, Jakub Jelinek wrote: On Thu, Dec 01, 2016 at 11:43:19PM -0700, Jeff Law wrote: Martin's alloca work flagged this code as problematical. Essentially if we had a statement with no operands and the statement was not in the hash table, then we could end up performing alloca (0), which is inadvisable. I still don't understand why it is inadvisable. alloca(0) is not undefined behavior. It can return NULL, or non-unique pointer, or a unique pointer, and/or cause freeing of already left alloca blocks (like any other alloca call). None of that is a problem here. If num is 0, then copy is just set and never used. I expect most if not all gcc uses of alloca where the count can be 0 are like that. It won't cause any problems in this and probably most instances, but leaving the code in its prior state is simply wrong from a maintenance standpoint. I'd much rather have the code explicitly and safely handle the zero operands case so that if someone makes a change later they don't have to worry about whether or not they're accessing memory which was never allocated. Additionally, it removes a false positive from the warning, thus making less noise. It's not unlike the strictly unnecessary initializations we do to shut up -Wuninitialized. Jeff
Re: Avoid alloca(0) when temporarily propagating operands during threading
On Thu, Dec 01, 2016 at 11:43:19PM -0700, Jeff Law wrote: > > Martin's alloca work flagged this code as problematical. Essentially if we > had a statement with no operands and the statement was not in the hash > table, then we could end up performing alloca (0), which is inadvisable. I still don't understand why it is inadvisable. alloca(0) is not undefined behavior. It can return NULL, or non-unique pointer, or a unique pointer, and/or cause freeing of already left alloca blocks (like any other alloca call). None of that is a problem here. If num is 0, then copy is just set and never used. I expect most if not all gcc uses of alloca where the count can be 0 are like that. Jakub
