[cfe-dev] Static check on memcpy()

[Richard Smith via cfe-dev]

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/20201203/808a4739/attachment.html>