Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list] 
Date: Fri, 14 Apr 2017 17:40:37 +0200
From: Florian Weimer <fw@...eb.enyo.de>
To: "Jason A. Donenfeld" <Jason@...c4.com>
Cc: oss-security@...ts.openwall.com
Subject: Re: alloca in inline functions can be dangerous

* Jason A. Donenfeld:

> Hey folks,
>
> I'm not sure this is the right mailing list to discuss this matter, but
> hopefully it finds an audience here. I was debugging some code recently,
> when I found a very nasty interaction between alloca and inline functions.
> Observe the following block:
>
> static inline void process_widget(struct widget *widget,
> 				  unsigned int  fcount)
> {
> 	struct fragment fragments[fcount];
> 	widgets_to_fragments(fragments, widget);
> 	process_fragments(fragments);
> }
>
> static void iterate_widgets(struct widgetlist *widgetlist)
> {
> 	struct widget *widget;
> 	unsigned int fcount;
>
> 	for (widget = widgetlist->first; widget; widget = widget->next) {
> 		fcount = widget_get_frags_required(widget);
> 		if (fcount > 256)
> 			continue;
> 		process_widget(widget, fcount);
> 	}
> }
>
> This seems pretty benign. However, let's look at two transformations that
> gcc makes. First the VLA is changed to use alloca:

Which GCC version do you use?

I turned your example into something that actually compiles:

struct widget
{
  struct widget *next;
};

unsigned widget_get_frags_required(struct widget *);

struct fragment { int dummy; };
void widgets_to_fragments(struct fragment *, struct widget *);
void process_fragments(struct fragment *);

struct widgetlist
{
  struct widget *first;
};

static inline void
process_widget(struct widget *widget, unsigned int  fcount)
{
  struct fragment fragments[fcount];
  widgets_to_fragments(fragments, widget);
  process_fragments(fragments);
}

void
iterate_widgets(struct widgetlist *widgetlist)
{
  struct widget *widget;
  unsigned int fcount;

  for (widget = widgetlist->first; widget; widget = widget->next) {
    fcount = widget_get_frags_required(widget);
    if (fcount > 256)
      continue;
    process_widget(widget, fcount);
  }
}

I don't see the behavior your outline.  The stack pointer is restored
for each iteration of the loop.

In my experience, GCC is pretty good at actually deallocating VLAs
when a scope is exited.

In any case, this is a GCC bug.  I can help you to turn this into a
proper bug report if it still exists in current versions.

Powered by blists - more mailing lists

Please check out the Open Source Software Security Wiki, which is counterpart to this mailing list.

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.