Bug 71445 - libsanitizer build failure on aarch64-linux-gnu with recent glibc
Summary: libsanitizer build failure on aarch64-linux-gnu with recent glibc
Status: RESOLVED WONTFIX
Alias: None
Product: gcc
Classification: Unclassified
Component: sanitizer (show other bugs)
Version: unknown
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-06-07 14:31 UTC by Yvan Roux
Modified: 2016-06-19 07:10 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments
sanitizer-msghdr.patch (906 bytes, patch)
2016-06-10 14:47 UTC, Jakub Jelinek
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yvan Roux 2016-06-07 14:31:35 UTC
Recent changes in Glibc to follow POSIX specifications for both msghdr and cmsghdr (switch from size_t to sockelen_t for msg_iovlen, msg_controllen, and cmsg_len on 64-bits arch.), libsanitizer fails to build for AArch64 (logs at the end), and I guess other 64-bits targets.

This issue was fixed in compiler-rt sources at revision 272008:

http://llvm.org/viewvc/llvm-project?view=revision&revision=272008

For information Glibc revision which introduced the issue is:

https://sourceware.org/git/?p=glibc.git;a=commit;h=222c2d7f4357d66073176f3beec67af40f0486c7

Full log of the failure:

.../libsanitizer/sanitizer_common/sanitizer_internal_defs.h:254:72:
error: size of array 'assertion_failed__1007' is negative
     typedef char IMPL_PASTE(assertion_failed_##_, line)[2*(int)(pred)-1]
                                                                        ^
.../libsanitizer/sanitizer_common/sanitizer_internal_defs.h:248:30:
note: in expansion of macro 'IMPL_COMPILER_ASSERT'
 #define COMPILER_CHECK(pred) IMPL_COMPILER_ASSERT(pred, __LINE__)
                              ^~~~~~~~~~~~~~~~~~~~
.../libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h:1410:3:
note: in expansion of macro 'COMPILER_CHECK'
   COMPILER_CHECK(sizeof(((__sanitizer_##CLASS *) NULL)->MEMBER) == \
   ^~~~~~~~~~~~~~
.../libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.cc:1007:1:
note: in expansion of macro 'CHECK_SIZE_AND_OFFSET'
 CHECK_SIZE_AND_OFFSET(msghdr, msg_iovlen);
 ^~~~~~~~~~~~~~~~~~~~~
.../libsanitizer/sanitizer_common/sanitizer_internal_defs.h:254:72:
error: size of array 'assertion_failed__1009' is negative
     typedef char IMPL_PASTE(assertion_failed_##_, line)[2*(int)(pred)-1]
                                                                        ^
.../libsanitizer/sanitizer_common/sanitizer_internal_defs.h:248:30:
note: in expansion of macro 'IMPL_COMPILER_ASSERT'
 #define COMPILER_CHECK(pred) IMPL_COMPILER_ASSERT(pred, __LINE__)
                              ^~~~~~~~~~~~~~~~~~~~
.../libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h:1410:3:
note: in expansion of macro 'COMPILER_CHECK'
   COMPILER_CHECK(sizeof(((__sanitizer_##CLASS *) NULL)->MEMBER) == \
   ^~~~~~~~~~~~~~
.../libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.cc:1009:1:
note: in expansion of macro 'CHECK_SIZE_AND_OFFSET'
 CHECK_SIZE_AND_OFFSET(msghdr, msg_controllen);
 ^~~~~~~~~~~~~~~~~~~~~
.../libsanitizer/sanitizer_common/sanitizer_internal_defs.h:254:72:
error: size of array 'assertion_failed__1013' is negative
     typedef char IMPL_PASTE(assertion_failed_##_, line)[2*(int)(pred)-1]
                                                                        ^
.../libsanitizer/sanitizer_common/sanitizer_internal_defs.h:248:30:
note: in expansion of macro 'IMPL_COMPILER_ASSERT'
 #define COMPILER_CHECK(pred) IMPL_COMPILER_ASSERT(pred, __LINE__)
                              ^~~~~~~~~~~~~~~~~~~~
.../libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h:1410:3:
note: in expansion of macro 'COMPILER_CHECK'
   COMPILER_CHECK(sizeof(((__sanitizer_##CLASS *) NULL)->MEMBER) == \
   ^~~~~~~~~~~~~~
.../libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.cc:1013:1:
note: in expansion of macro 'CHECK_SIZE_AND_OFFSET'
 CHECK_SIZE_AND_OFFSET(cmsghdr, cmsg_len);
 ^~~~~~~~~~~~~~~~~~~~~
Comment 1 Maxim Ostapenko 2016-06-07 16:48:09 UTC
Hm, one I can see, Glibc still has __GLIBC_MINOR__ = 23:

/* Major and minor version number of the GNU C library package.  Use
   these macros to test for features in specific releases.  */
#define __GLIBC__       2
#define __GLIBC_MINOR__ 23

Perhaps we can cherry-pick 
http://llvm.org/viewvc/llvm-project?view=revision&revision=272008 after Glibc release happens?
Comment 2 Adhemerval Zanella 2016-06-07 17:33:16 UTC
This is not really aarch64 also, all 64-bit architectures will require this fix.  And indeed, the features.h header is only fixed at release, until them GLIBC sets its version to '2.23.90' (meaning 2.23 development).
Comment 3 Yvan Roux 2016-06-08 08:02:18 UTC
I'd prefer to cherry-pick it now as it'll not change the current behavior and simplify the job for the ones (like us ;) who monitor Glibc/GCC trunk developments (not that cherry-picking the fix is complicated, but the less we modify the sources we validate the better it is).  But I can also understand that you prefer to wait Glibc 2.24 before doing it.
Comment 4 Jakub Jelinek 2016-06-08 09:40:56 UTC
That change looks wrong to me though.
If glibc has added symbol versioning and changed ABI for existing symbols that sanitizer libraries wrap, then I'm afraid either we need to start symbol versioning libsanitizer too (and make sure to use the same symbol version glibc uses for the various symbols in there, so e.g. on x86_64 libasan would need to export recvmsg@GLIBC_2.2.5 and recgmsg@@GLIBC_2.24), or the wrappers need to be changed (if at all possible), so that they provide ABI compatibility, so that it will work well for apps linked against the pre-glibc-2.24 {recvmsg,sendmsg,recvmmsg,sendmmsg} (whatever else changed) as well as apps linked agains the new one.  I doubt a plain change in the structure layout is sufficient here.
Comment 5 Maxim Ostapenko 2016-06-08 09:51:50 UTC
Can we use dlvsym for versioned symbols (recvmsg, sendmsg, etc) in the wrappers?
Comment 6 Jakub Jelinek 2016-06-08 10:15:45 UTC
I don't see how that would help much.  You still wouldn't know if the recvmsg or sendmsg that is being called by whatever library and is interposed by libasan etc. is the one with 64-bit msg_iovlen, msg_controllen or cmsg_len, or 32-bit one.
E.g. in write_msghdr used by recvmsg wrapper, there is:
  if (msg->msg_iov && msg->msg_iovlen)
    COMMON_INTERCEPTOR_WRITE_RANGE(ctx, msg->msg_iov,
                                   sizeof(*msg->msg_iov) * msg->msg_iovlen);
  write_iovec(ctx, msg->msg_iov, msg->msg_iovlen, maxlen);
The broken change committed to llvm repo makes this a static choice - if you compile libsanitizer against glibc older than 2.24, you assume msg_iovlen must be always 64-bit, even if you e.g. at runtime link against glibc 2.24 and run a program that uses recvmsg@@GLIBC_2.24.
If you compile libsanitizer against glibc 2.24+, you assume msg_iovlen must be always 32-bit, even if the app calls recvmsg@GLIBC_2.2.5.  Not sure what dlsym actually gives you in that case, I think recvmsg@@GLIBC_2.24, but it would need to be verified.
What we could do is just assume people don't really use > 2G values in these cases, and just use (int) msg->msg_iovlen instead of msg->msg_iovlen (for Linux only).  Similarly for the other two fields, and perhaps keep the structures as is and just make the size and offset checks for these fields accept all the expected variants (i.e. also the case where the glibc headers have the field half the size and for big endian also at a different offset (difference of half the size).
For e.g. asan, this would just mean that programs linked against glibc < 2.24 that use > 2GB msg_iovlen and overwrite through that some buffer wouldn't be detected, I think that is sufficiently rare that it might be acceptable.
Over time more and more programs/libraries will be rebuilt against glibc >= 2.24 and the problem will go away there.
Comment 7 Yuri Gribov 2016-06-08 11:44:15 UTC
Same issue: https://github.com/google/sanitizers/issues/654
Comment 8 Adhemerval Zanella 2016-06-08 14:25:05 UTC
Indeed I did not take in consideration the versioned issue with interposed wrapper mainly because the idea of the patch was to fix the static compile asserts against newer glibc.  I see the runtime issue as separated issue.

However regarding the kernel interface, kernel accepts 64-bit msg_iovlen, msg_controllen and cmsg_len; so it is a matter if libsanitizer also want to enforce the POSIX compliance in the syscalls for Linux.  One solution is as Jakub suggested is limit the check to 32-bits values, another might be to remove the compiler asserts and use the kernel interface instead of defined POSIX one. I am not sure which is better option.
Comment 9 Jakub Jelinek 2016-06-08 14:32:38 UTC
The thing is that programs or libraries compiled/linked against glibc 2.24+ might contain garbage in the high 32 bits of those values.
So using e.g. kernel APIs directly in the wrappers will misbehave and so would the asan etc. diagnostics.
I really think we either should symbol version all of libsanitizer (but the wrappers around libc functions should use the symbol versions from libc, so it will be non-fun, because the exact versions differ between different architectures), or always ignore the high bits, or at runtime check if we are running against glibc 2.24+ and only ignore the high bits in that case (the compatibility with pre-2.24 compiled objects would still not be perfect, as we'd ignore the upper bits when running against glibc 2.24+, but when running against older glibc we'd honor those).
Comment 10 Adhemerval Zanella 2016-06-08 15:02:20 UTC
I think add versioned symbols in libsanitizer seems feasible with current supported platforms and seems to be the more complete fix. I will check on that.
Comment 11 Jakub Jelinek 2016-06-09 09:09:14 UTC
True; on the other side, at least for backports we need some other solution, because the symbol versioning stuff can't be backported to GCC 6.2 or 5.5 and people will surely want to be able to compile those GCCs against more recent glibcs.
Comment 12 Adhemerval Zanella 2016-06-09 13:00:34 UTC
After checking the work required for enable symbol versioning wrapper on libsanitizer I am more inclined to go with always ignore the high bits (at least for backports). The versioning enablement will require some changes and I foresee it will generate some discussion in sanitizer side before a settle solution is used. Any suggestions?
Comment 13 Carlos O'Donell 2016-06-09 15:43:48 UTC
(In reply to Adhemerval Zanella from comment #12)
> After checking the work required for enable symbol versioning wrapper on
> libsanitizer I am more inclined to go with always ignore the high bits (at
> least for backports). The versioning enablement will require some changes
> and I foresee it will generate some discussion in sanitizer side before a
> settle solution is used. Any suggestions?

+1

That sounds like the most sensible course of action.

The real solution IMO is for the interposer to provide all the various versions of a function and sanitize them each as needs be. There is no other way around this and this was a problem that was going to happen some day, we are just having it happen today because of the changes in glibc.
Comment 14 Jakub Jelinek 2016-06-10 14:47:05 UTC
Created attachment 38682 [details]
sanitizer-msghdr.patch

Completely untested patch with what I had in mind.
Seems cmsg_len is not really used in any interceptors (other than being checked for size and offset), and while msg_iovlen and msg_controllen are, they are never written to by libsanitizer.  There is sanitizer_common/sanitizer_common_syscalls.inc
which uses the kernel structure and thus is fine in any case.
By always using just the low 32 bits of msg_iovlen and msg_controllen for 64-bit Linux, all that happens is that some COMMON_INTERCEPTOR_WRITE_RANGE might not be performed or would be shorter than for the POSIX violating structure layout.
I guess that is a reasonable price to pay.
But, as unlike msg_controllen and cmsg_len, msg_iovlen is signed integer, I wonder if we also shouldn't change:
if (msg->msg_iov && msg->msg_iovlen)
in write_msghdr to:
if (msg->msg_iov && msg->msg_iovlen > 0)
and add
if (msg->msg_iovlen > 0)
guard before the write_iovec call.
But, I must say all of write_msghdr looks very weird to me.
I thought the syscall only writes the msg_flags field of the structure itself upon success, all the other fields are read, so
COMMON_INTERCEPTOR_WRITE_RANGE(ctx, msg, sizeof(*msg));
looks bogus to me, I'd have expected COMMON_INTERCEPTOR_READ_RANGE on the selected fields that are known to be read, and COMMON_INTERCEPTOR_WRITE_RANGE in write_msghdr only for &msg->msg_flags.  Similarly, I believe what msg->msg_iov points to is only read, never written, so again I think
COMMON_INTERCEPTOR_WRITE_RANGE(ctx, msg->msg_iov, ...
should have been
COMMON_INTERCEPTOR_READ_RANGE, and most likely before the call rather than in write_msghdr.
Comment 15 Florian Weimer 2016-06-10 14:48:46 UTC
There is now a proposal to revert the glibc change instead:

  https://sourceware.org/ml/libc-alpha/2016-06/msg00339.html
Comment 16 Adhemerval Zanella 2016-06-10 14:55:49 UTC
I am sorry for make you waste your time Jakub, after some deliberation I decided to revert this patch on glibc.  I will close this bug after push this change on glibc side.
Comment 17 Jakub Jelinek 2016-06-10 14:59:07 UTC
Well, you should then revert your compiler-rt patch (and most likely double check the signed vs. unsigned types, in the patch you've committed there were various signedness (but for big endian even size) issues).
And, somebody from the sanitizer folks should have look at the write_msghdr/recgmsg sanitizer wrapper issues I've raised.
Comment 18 Adhemerval Zanella 2016-06-10 15:07:24 UTC
Yes, I will take care of reverting compiler-rt changes and bring the issues you raised.
Comment 19 Andrew Pinski 2016-06-19 07:10:01 UTC
Won't fix.  That is glibc patch was reverted.