[libc++] remove possible trailing padding from aligned_storage

[yiranwang]

Summary

In libc++, there are some usage of aligned_storage which uses "sizeof" bytes of raw data. This is problematic a bit, as the trailing padding area will be counted by "sizeof", and it leads to out of bound access. For example, the member __buf_ of std::function can be used to store pointers to parameters, and the compiler could fail to figure out there is a pointer in the padding area points to some local variable.
The fix enlarges the buffer so that the size is exact multiple of "_Align". It is of no run time overhead.

[danalbert]

FYI this was found because it can cause issues when used with GCC.

[EricWF]

I'm not sure I understand the problem. Is it possible to provide a minimal reproducer of the bug/behavior you are describing?

[yiranwang]

A test case is as following. It has to be build by GCC 4.9 -O3 (maybe or later), with latest libc++, and for AARCH64+ANDROID target.
AARCH64 requires 128 bit alignment for aligned_storage and 64 bit pointers, while gcc 4.9 alias analysis will do field-sensitive points-to analysis. But this could happen for other ISA+ABI.
The fundamental issue is that for this combination, std::function has member buf_ declared as
aligned_storage<3*sizoef(void*)>::type buf_;
Basically, it is
aligned_storage<24>::type;
This will generate aligned_storage of, _Len==24 and _Align==16;
While std::function will use the buf_ to sizeof(buf_) bytes (at line 1593 and 1628 of <functional>), which is 32. Basically, the pointer to "tbool' will be stored at "&__buf_+24".
This is not a well defined memory area, and GCC alias analysis is going to ignore the "ESCAPE" of address of "tbool". Basically, the function "test_simple" would always return "false".

#include <functional>
extern void external_test(std::function<bool()>fn);
extern bool test_simple(){

bool tbool = false;
int a, b;
external_test([&a, &b, &tbool](){
  tbool = true;
  return true;
});
return tbool;

}

[davidxl]

In libc++, placement new is used in many places. When selecting the buffer size for the placed object, it uses the 'actual' size of the buffer including the padding bytes from alignment, instead of the declared of the buffer. As a result, the declared buffer size may be smaller than the target object. Due to this mismatch, the compiler may see out of bound access of the buffer thus miscompile the program.

The purpose of the fix is to make the declared size == actual buffer size.

[EricWF]

I think I'm starting to understand but I think the correct fix for this would be to fix function so that:

1. It aligns its buffer by alignof(void*).
2. It doesn't stick over-aligned types in the small object buffer.

Point two can probably be done without an ABI break, but point one will break the ABI.

[davidxl]

We certainly need a fix without breaking ABI. Is there a ABI conformance test for libcxx?

[EricWF]

In D12247#233323, @davidxl wrote:

We certainly need a fix without breaking ABI. Is there a ABI conformance test for libcxx?

Well this patch definitely breaks the ABI. This was my attempt at fixing the problem in std::function that might not be ABI breaking..
https://gist.github.com/EricWF/3a35b140a66d4826a9d0.

[EricWF]

In D12247#233323, @davidxl wrote:

We certainly need a fix without breaking ABI. Is there a ABI conformance test for libcxx?

To actually answer @davidxl's question: No. libc++ does not have an ABI test suite of any sort.

[yiranwang]

In D12247#233349, @EricWF wrote:

In D12247#233323, @davidxl wrote:

We certainly need a fix without breaking ABI. Is there a ABI conformance test for libcxx?

Well this patch definitely breaks the ABI. This was my attempt at fixing the problem in std::function that might not be ABI breaking..
https://gist.github.com/EricWF/3a35b140a66d4826a9

I think following your thought, we should abandon the usage of "aligned_storage" in "function" totally. Instead, just say "void *buf_[3];", and check alignment_of _Fp (can be arbitrarily big, but usually small), and sizeof the two to guard use of buf_. The alignment bump to 16 by aligned_storage on AARCH64 is not what we want, performance wise.

Also, such kind of change need to applied for some more places, at least in libc++, such like <libcxx/include/future>

[yiranwang]

some more explanation of last comment.
For most architecture sizeof(function) is 4*sizeof(void *), but it is 6*sizeof(void*) on AARCH64/LINUX, which does not sound so great as there are padding of 2*sizeof(void*) bytes.

[EricWF]

In D12247#233595, @yiranwang wrote:

In D12247#233349, @EricWF wrote:

In D12247#233323, @davidxl wrote:

We certainly need a fix without breaking ABI. Is there a ABI conformance test for libcxx?

Well this patch definitely breaks the ABI. This was my attempt at fixing the problem in std::function that might not be ABI breaking..
https://gist.github.com/EricWF/3a35b140a66d4826a9

I think following your thought, we should abandon the usage of "aligned_storage" in "function" totally. Instead, just say "void *buf_[3];", and check alignment_of _Fp (can be arbitrarily big, but usually small), and sizeof the two to guard use of buf_. The alignment bump to 16 by aligned_storage on AARCH64 is not what we want, performance wise.

Sort of. I wouldn't abandon the use of aligned_storage in function but I would change it from aligned_storage<3*sizeof(void*)>::type to aligned_storage<3*sizeof(void*), alignof(void*)>::type. That would have eventually the same behavior your describing.

The reason that I haven't made that change is that it is ABI breaking.

Also, such kind of change need to applied for some more places, at least in libc++, such like <libcxx/include/future>

Yes it would.

[yiranwang]

Hi Eric,

Could you please explain a bit more what is broken specifically? As we can see, sizeof(), _Len, and _Align, and alignof() of "aligned_storage" are all not changed.

[EricWF]

In D12247#233717, @yiranwang wrote:

Hi Eric,

Could you please explain a bit more what is broken specifically? As we can see, sizeof(), _Len, and _Align, and alignof() of "aligned_storage" are all not changed.

That's correct. At the risk of sounding like a broken record those fields have not changed because doing so would break the ABI. Instead my patch fixes the issue your seeing by simply not using __buf_ unless its the correct size and correctly aligned.

The alignment is the important part. Previously we didn't check if Fn was alignment compatible with __buf_.

[EricWF]

In D12247#231444, @yiranwang wrote:

A test case is as following. It has to be build by GCC 4.9 -O3 (maybe or later), with latest libc++, and for AARCH64+ANDROID target.
AARCH64 requires 128 bit alignment for aligned_storage and 64 bit pointers, while gcc 4.9 alias analysis will do field-sensitive points-to analysis. But this could happen for other ISA+ABI.
The fundamental issue is that for this combination, std::function has member buf_ declared as
aligned_storage<3*sizoef(void*)>::type buf_;
Basically, it is
aligned_storage<24>::type;
This will generate aligned_storage of, _Len==24 and _Align==16;
While std::function will use the buf_ to sizeof(buf_) bytes (at line 1593 and 1628 of <functional>), which is 32. Basically, the pointer to "tbool' will be stored at "&__buf_+24".
This is not a well defined memory area, and GCC alias analysis is going to ignore the "ESCAPE" of address of "tbool". Basically, the function "test_simple" would always return "false".

#include <functional>
extern void external_test(std::function<bool()>fn);
extern bool test_simple(){

bool tbool = false;
int a, b;
external_test([&a, &b, &tbool](){
  tbool = true;
  return true;
});
return tbool;

}

What GCC flags do I need to build it in this configuration?

[EricWF]

I think I've misunderstood this patch the whole time. I didn't understand that it was undefined to access the trailing padding of the storage type. Am I correct in saying that this patch won't change the actual values of sizeof(aligned_storage<N>::type) and sizeof(aligned_storage<N>::type) for any N? I thought this was changing the type sizes but it's really just making trailing padding part of the actual type.

We probably still need part of my change for std::function but I also think we need this patch.

[davidxl]

yes -- the intention of Yiran's patch is not to change the size/alignment of the underlying storage, but to make the original padding space part of the type.

[EricWF]

So this patch LGTM. Sorry for being slow to understand it. However I want to see two things before it lands.

1. I would like to see a test of some sort that the resulting type has the same size and alignment requirements it did before the change. I'm not sure what the best way to test this is.
2. I would like @mclow.lists to approve this as well. I just want to be cautious.

[yiranwang]

In D12247#234533, @EricWF wrote:

So this patch LGTM. Sorry for being slow to understand it. However I want to see two things before it lands.

1. I would like to see a test of some sort that the resulting type has the same size and alignment requirements it did before the change. I'm not sure what the best way to test this is.
2. I would like @mclow.lists to approve this as well. I just want to be cautious.

I think now we are on the same page about these bugs.
Also, thanks for your review and findings (hopefully fix it soon too).

For the test, we compile it with -O3 -fstrict-aliasing -finline-limit=300, and as mentioned before the target is AARCH64+ANDROID.

[EricWF]

@mclow.lists I have also tested this patch using libabigail and it couldn't spot any problems.

[yiranwang]

Thanks for testing.
There is proof as the following, given that alignment of _Aligner is _Align (the whole purpose of _Aligner here, which we do not want to double check), and for any type, the "sizeof" is always multiple of "alignof", this change should be ABI compatible.
Are we good to go?

[EricWF]

LGTM. I don't want to hold this up any longer. @mclow.lists can comment on this after it's committed if he sees any issues.

[yiranwang]

Thank you, Eric. Also, could you please help to commit the change? I personally do not have the permissions to change libc++ code, thanks a lot.

[EricWF]

Sure sorry for the delay.

[EricWF]

Committed as r248309.

include/type_traits

     union type
     {
         _Aligner __align;
-        unsigned char __data[_Len];
+        unsigned char __data[(_Len + _Align - 1)/_Align * _Align];
     };
 };
 

 {\
     struct _ALIGNAS(n) type\
     {\
-        unsigned char __lx[_Len];\
+        unsigned char __lx[(_Len + n - 1)/n * n];\
     };\
 }