Bug 197060 – WebCore: auto-initialize stack variables

• Component: WebCore Misc.
• Status: ASSIGNED

[JF Bastien]

Clang added support for C / C++ variable auto-initialization.

The build’s CFLAGS / CCFLAGS can use the following flag:
  -ftrivial-auto-var-init=pattern
An Xcode setting can also be used, which will enable said flag if it's available in clang.

Variable auto-initialization will affect stack variables as follows:
 - Initialize all integers and pointers to repeated 0xAA byte pattern
 - Initialize all floating point values to “negative NaN with repeated 0xFF payload” (i.e. float32 is 0xFFFFFFFF and float64 is 0xFFFFFFFFFFFFFFFF)
 - Initialize aggregate types in the same way, and their padding (structs, classes, vectors, arrays, variable-length arrays, unions)
The optimizer will then get rid of redundant initializations (if it doesn't, please file bugs on clang).

This means that attackers can’t exploit uninitialized stack variables as easily because they can’t leave a value on the stack / in a register and use it later in a manner which the program wasn’t designed to handle.

This currently increases binary size by 1%–2% and, in some cases, has a similar or smaller performance impact.We’ve put effort in reducing this impact, but the compiler can still be improved to reduce this impact. Concretely, WebCore on x86-64 grows by 1.9% on a recent clang (though we've done more optimizations since this measurement). We ran performance numbers on benchmarks we find important and Dean said "looking at the results, I think we have enough evidence to say it doesn't appear to be a regression".

As a workaround when performance impact is unacceptable, the compiler isn't optimizing things away, and you've manually validated that code was correct, you can mark the corresponding stack variable as follows:
  struct uninitialized_struct __attribute((uninitialized));
  int uninitalized_array[42] __attribute((uninitialized));
  int uninitialized_scalar __attribute((uninitialized));
Of course, you then have no protection of that variable from uninitialized usage!

[JF Bastien]

<rdar://problem/50018087>

[JF Bastien]

<rdar://problem/50018087>

[Radar WebKit Bug Importer]

<rdar://problem/50018144>

[Radar WebKit Bug Importer]

<rdar://problem/50018155>

[JF Bastien]

Created attachment 367730 [details]
patch

[Alexey Proskuryakov]

Comment on attachment 367730 [details]
patch

Please add a ChangeLog. r- on this technicality. Also some questions and suggestions.

1. I can see how this can be somewhat perf neutral on optimized builds, but what does this do to debug builds? We don't want tests to become slower in debug mode.

2. How does this interact with UBSan, will we still be getting diagnostics for uninitialized variables? I think that it's still a net win even if this breaks this aspect of UBSan functionality, but it would be useful to know how exactly it works.

3. Similarly, how does this interact with static analysis, will clang static analyzer now think that all these variables are initialized?

4. For the 1.9% WebCore size regression, I'm assuming that all of that are additional initializations, so it's quite surprising that so much new code isn't registering on benchmarks. Is there some explanation for that?

5. There was a discussion about initializing all variables by default recently in bug 196855, with regards to silencing Coverity warnings. The idea that it's OK to introduce some perf regressions as long as our current benchmark suite can't detect those is not universally agreed upon. The right place to discuss that is the webkit-dev mailing list.

[JF Bastien]

Created attachment 367734 [details]
patch

Add changelog

[JF Bastien]

(In reply to Alexey Proskuryakov from comment #6)
> Comment on attachment 367730 [details]
> patch
> 
> Please add a ChangeLog. r- on this technicality. Also some questions and
> suggestions.

Done.


> 1. I can see how this can be somewhat perf neutral on optimized builds, but
> what does this do to debug builds? We don't want tests to become slower in
> debug mode.

It generates more code in debug builds, but we didn't measure the performance impact. If debug performance matters I suggest not just using -O0 because it's quite terrible. There's a handful of optimizations that you could turn on that would compile *faster* or slightly slower (depends which) while still generating totally debuggable code. There's also ongoing work to create a better low-optimization pipeline in LLVM. I think this discussion should be held separately from this patch.


> 2. How does this interact with UBSan, will we still be getting diagnostics
> for uninitialized variables? I think that it's still a net win even if this
> breaks this aspect of UBSan functionality, but it would be useful to know
> how exactly it works.

It doesn't interact with UBSan. UBSan also doesn't give you diagnostics for uninitialized variables. Only msan does this for stack values.


> 3. Similarly, how does this interact with static analysis, will clang static
> analyzer now think that all these variables are initialized?

It doesn't interact with static analysis (nor the warnings). The diagnostics remain unchanged.


> 4. For the 1.9% WebCore size regression, I'm assuming that all of that are
> additional initializations, so it's quite surprising that so much new code
> isn't registering on benchmarks. Is there some explanation for that?

Yes, I've sent detailed information to the folks I've CC'ed to the review. Some is caused by large stack buffers, some is new code (when memset / memcpy gets inlined), and some is rodata globals that get memcpy'd to the stack (because that particular stack variable is big and isn't a repeated byte pattern). This in turn influences inlining decisions.

If you want all the details please look at the related radars, I've done extensive measurements and optimizations. Keep in mind that there's still ongoing work to optimize this further (by myself and others in the LLVM project).


> 5. There was a discussion about initializing all variables by default
> recently in bug 196855, with regards to silencing Coverity warnings. The
> idea that it's OK to introduce some perf regressions as long as our current
> benchmark suite can't detect those is not universally agreed upon. The right
> place to discuss that is the webkit-dev mailing list.

I don't follow webkit-dev, but I suggest reading the radar and related ones for the discussions that have happened.

[Alexey Proskuryakov]

Comment on attachment 367734 [details]
patch

> I don't follow webkit-dev, but I suggest reading the radar and related ones for the discussions that have happened.

What I'm saying is that we shouldn't make such policy changes without a webkit-dev discussion.

[Alexey Proskuryakov]

> It doesn't interact with UBSan. UBSan also doesn't give you diagnostics for uninitialized variables. Only msan does this for stack values.

OK. Does it break MSan uninitialized variable diagnostics? If it doesn't explicitly interact with it, it probably does break it, as variables are now initialized.

[JF Bastien]

(In reply to Alexey Proskuryakov from comment #10)
> > It doesn't interact with UBSan. UBSan also doesn't give you diagnostics for uninitialized variables. Only msan does this for stack values.
> 
> OK. Does it break MSan uninitialized variable diagnostics? If it doesn't
> explicitly interact with it, it probably does break it, as variables are now
> initialized.

This patch turns on auto-init for platforms that build with Xcode. Which of those do you run msan on?

[Alexey Proskuryakov]

Currently none, but we are very interested in enabling diagnostics of this kind.