[cfe-dev] Static check on memcpy()

[Richard Smith via cfe-dev]

On Fri, 4 Dec 2020 at 06:47, Chris Hamilton <chris.hamilton at ericsson.com>
wrote:

> Quoth Richard Smith:
>
>
>
> In this particular case, where we can statically see that the size of the
> source is sizeof(struct S), it seems like this is something we could easily
> warn on, using much the same logic that FORTIFY_SOURCE would use at
> runtime: effectively, compute `__builtin_object_size(&src)` and compare
> that to `sizeof(struct S) + 1`. While that seems straightforward enough,
> it's only going to work in the cases where the pointer arguments are simple
> enough that we can constant-evaluate them in the frontend. Is that general
> enough for what you're looking for? (I think as a general strategy,
> implementing warnings for all the cases that FORTIFY_SOURCE would trap on
> at runtime would be a great idea.)
>
>
>
> So, to be sure I understand, if instead of this:
>
>
>
>   memcpy(dest, &src, sizeof(struct S) + 1);
>
>
>
> we had this:
>
>
>
>   unsigned famLen = …;  // some passed parameter or computed value
>
>   memcpy(dest, &src, sizeof(struct S) + famLen);
>
>
>
> then, the FE would not be able to warn about this because ‘sizeof(struct
> S) + famLen’ cannot be constant-evaluated?
>

Yes. (But the static analyzer would have no problem with such things.)


>  Chris
>

>
>
>
> *From:* Richard Smith <richard at metafoo.co.uk>
> *Sent:* Thursday, December 3, 2020 6:03 PM
> *To:* Chris Hamilton <chris.hamilton at ericsson.com>
> *Cc:* cfe-dev at lists.llvm.org
> *Subject:* Re: [cfe-dev] Static check on memcpy()
>
>
>
> On Thu, 3 Dec 2020 at 14:34, Chris Hamilton via cfe-dev <
> cfe-dev at lists.llvm.org> wrote:
>
> Hi folks,
>
>
>
> It’s easy to see how memcpy (and other mem* functions) can cause
> out-of-bounds reads/writes, such as in this simplified reproducer for a
> real case we’ve seen:
>
>
>
>    #include <string.h>
>
>    struct S {
>
>       int x;
>
>       int xx;
>
>       int y[];
>
>    };
>
>    char dst[100];
>
>
>
>    int main(void) {
>
>       struct S src = {0};
>
>       src.x = 9999;
>
>       src.xx = 8888;
>
>       memcpy(dst, &src, sizeof(struct S) + 1);
>
>    }
>
>
>
> Here, the size argument to memcpy is clearly just wrong.  But consider
> that when FAMs are in play (as is hinted at here), designers can get
> confused and use the wrong size value – probably there are plenty of other
> circumstances where such coding errors are easy to make, and not easy to
> spot during review.  At present, CFE can’t catch this during compilation
> (unless I’ve missed something).  It can be caught by the static analysis
> check “alpha.unix.cstring.OutOfBounds” – but that’s rather late, rather
> costly, and rather noisy (which I’m sure is why it’s an alpha check and not
> a core check).  This seems like something that could be caught and flagged
> by either a diagnostic or a tidy-check…   Is that reasonable?  If not, why
> not?
>
>
>
> In this particular case, where we can statically see that the size of the
> source is sizeof(struct S), it seems like this is something we could easily
> warn on, using much the same logic that FORTIFY_SOURCE would use at
> runtime: effectively, compute `__builtin_object_size(&src)` and compare
> that to `sizeof(struct S) + 1`. While that seems straightforward enough,
> it's only going to work in the cases where the pointer arguments are simple
> enough that we can constant-evaluate them in the frontend. Is that general
> enough for what you're looking for? (I think as a general strategy,
> implementing warnings for all the cases that FORTIFY_SOURCE would trap on
> at runtime would be a great idea.)
>
>
>
> Alternatively, if we could get a low-false-positive version of that static
> analyzer check promoted out of alpha, then that would be a more general
> solution (and would automatically give you a clang-tidy check for this).
> But it seems better to trap this during compilation rather than after, in
> the cases where we can.
>
>
>
> Regards,
>
>
>
> <https://www.ericsson.com/>
>
>
>
> *Chris Hamilton*
>
> Compiler Developer
>
> BNEW DNEW 4G5G BI BBI 10
>
> Mobile: +1-512-955-0143
>
> chris.hamilton at ericsson.com
>
>
>
> *“Without inclusion, diversity is only a statistic.” * *-- Börje Ekholm,
> CEO of Ericsson*
>
>
>
> Ericsson
>
> 1703 W. 5th Street Suite 600
>
> 78703,Austin, Texas
>
> United States
>
> ericsson.com <https://www.ericsson.com/>
>
>
>
> <https://www.ericsson.com/current_campaign>
>
>
>
> Our commitment to Technology for Good
> <https://www.ericsson.com/thecompany/sustainability-corporateresponsibility>
> and Diversity and Inclusion
> <https://www.ericsson.com/thecompany/diversity-inclusion> contributes to
> positive change.
> Follow us on: Facebook <https://www.facebook.com/ericsson> LinkedIn
> <https://www.linkedin.com/company/ericsson> Twitter
> <https://twitter.com/Ericsson>
>
> Legal entity:ERICSSON AB registration number 556056-6258, registered
> office in Stockholm.
> This communication is confidential. Our email terms:
> www.ericsson.com/en/legal/privacy/email-disclaimer
>
>
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20201207/545a5529/attachment-0001.html>