r281277 - [Sema] Fix PR30346: relax __builtin_object_size checks.

[George Burgess IV via cfe-commits]

Yeah, this patch didn't give me the warm fuzzies, either.

AFAICT, our only other options are having some sort of struct whitelist
(either hard-coded, or given as a flag), or telling people to turn
_FORTIFY_SOURCE down if they have code that looks like this. Given that
this is apparently common in the world of BSD, I'm unsure how many projects
this would impact if we chose "turn _FORTIFY_SOURCE down." So, this
solution seemed like the least bad of the above.

If people feel differently or have other ideas, I'm happy to add
restrictions/try a different approach. :)

On Tue, Sep 13, 2016 at 12:51 PM, Richard Smith via cfe-commits <
cfe-commits at lists.llvm.org> wrote:

> On Tue, Sep 13, 2016 at 10:44 AM, Joerg Sonnenberger via cfe-commits <
> cfe-commits at lists.llvm.org> wrote:
>
>> On Mon, Sep 12, 2016 at 11:50:36PM -0000, George Burgess IV via
>> cfe-commits wrote:
>> > Author: gbiv
>> > Date: Mon Sep 12 18:50:35 2016
>> > New Revision: 281277
>> >
>> > URL: http://llvm.org/viewvc/llvm-project?rev=281277&view=rev
>> > Log:
>> > [Sema] Fix PR30346: relax __builtin_object_size checks.
>> >
>> > This patch makes us act more conservatively when trying to determine
>> > the objectsize for an array at the end of an object. This is in
>> > response to code like the following:
>> >
>> > ```
>> > struct sockaddr {
>> >   /* snip */
>> >   char sa_data[14];
>> > };
>> >
>> > void foo(const char *s) {
>> >   size_t slen = strlen(s) + 1;
>> >   size_t added_len = slen <= 14 ? 0 : slen - 14;
>> >   struct sockaddr *sa = malloc(sizeof(struct sockaddr) + added_len);
>> >   strcpy(sa->sa_data, s);
>> >   // ...
>> > }
>> > ```
>> >
>> > `__builtin_object_size(sa->sa_data, 1)` would return 14, when there
>> > could be more than 14 bytes at `sa->sa_data`.
>> >
>> > Code like this is apparently not uncommon. FreeBSD's manual even
>> > explicitly mentions this pattern:
>> > https://www.freebsd.org/doc/en/books/developers-handbook/soc
>> kets-essential-functions.html
>> > (section 7.5.1.1.2).
>> >
>> > In light of this, we now just give up on any array at the end of an
>> > object if we can't find the object's initial allocation.
>> >
>> > I lack numbers for how much more conservative we actually become as a
>> > result of this change, so I chose the fix that would make us as
>> > compatible with GCC as possible. If we want to be more aggressive, I'm
>> > happy to consider some kind of whitelist or something instead.
>>
>> IMO this should be restricted to code that explicitly disables C/C++
>> aliasing rules.
>
>
> Do you mean -fno-strict-aliasing or -fno-struct-path-tbaa or something
> else here? (I think we're not doing anyone any favours by making
> _FORTIFY_SOURCE say that a pattern is OK in cases when LLVM will in fact
> optimize on the assumption that it's UB, but I don't recall how aggressive
> -fstruct-path-tbaa is for trailing array members.)
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160913/61d58ef8/attachment-0001.html>