<div dir="ltr"><div dir="ltr">On Fri, 4 Dec 2020 at 06:47, Chris Hamilton <<a href="mailto:chris.hamilton@ericsson.com">chris.hamilton@ericsson.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div lang="EN-US"><div class="gmail-m_6496434023715043067WordSection1"><p class="MsoNormal"><span style="color:rgb(31,73,125)">Quoth Richard Smith:<u></u><u></u></span></p><p class="MsoNormal"><span style="color:rgb(31,73,125)"><u></u> <u></u></span></p><p class="MsoNormal" style="margin-left:0.5in">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.)<u></u><u></u></p><p class="MsoNormal"><span style="color:rgb(31,73,125)"><u></u> <u></u></span></p><p class="MsoNormal"><span style="color:rgb(31,73,125)">So, to be sure I understand, if instead of this:<u></u><u></u></span></p><p class="MsoNormal"><span style="color:rgb(31,73,125)"><u></u> <u></u></span></p><p class="MsoNormal"><span style="color:rgb(31,73,125)">  memcpy(dest, &src, sizeof(struct S) + 1);<u></u><u></u></span></p><p class="MsoNormal"><span style="color:rgb(31,73,125)"><u></u> <u></u></span></p><p class="MsoNormal"><span style="color:rgb(31,73,125)">we had this:<u></u><u></u></span></p><p class="MsoNormal"><span style="color:rgb(31,73,125)"><u></u> <u></u></span></p><p class="MsoNormal"><span style="color:rgb(31,73,125)">  unsigned famLen = …;  // some passed parameter or computed value<u></u><u></u></span></p><p class="MsoNormal"><span style="color:rgb(31,73,125)">  memcpy(dest, &src, sizeof(struct S) + famLen);<u></u><u></u></span></p><p class="MsoNormal"><span style="color:rgb(31,73,125)"><u></u> <u></u></span></p><p class="MsoNormal"><span style="color:rgb(31,73,125)">then, the FE would not be able to warn about this because ‘sizeof(struct S) + famLen’ cannot be constant-evaluated?</span></p></div></div></blockquote><div><br></div><div>Yes. (But the static analyzer would have no problem with such things.)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div lang="EN-US"><div class="gmail-m_6496434023715043067WordSection1"><p class="MsoNormal"><span style="color:rgb(31,73,125)"> </span><span style="color:rgb(31,73,125)">Chris</span></p></div></div></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div lang="EN-US"><div class="gmail-m_6496434023715043067WordSection1"><p class="MsoNormal"><span style="color:rgb(31,73,125)"><u></u> <u></u></span></p><p class="MsoNormal"><span style="color:rgb(31,73,125)"><u></u> <u></u></span></p><div style="border-top:none;border-right:none;border-bottom:none;border-left:1.5pt solid blue;padding:0in 0in 0in 4pt"><div><div style="border-right:none;border-bottom:none;border-left:none;border-top:1pt solid rgb(225,225,225);padding:3pt 0in 0in"><p class="MsoNormal"><b>From:</b> Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> <br><b>Sent:</b> Thursday, December 3, 2020 6:03 PM<br><b>To:</b> Chris Hamilton <<a href="mailto:chris.hamilton@ericsson.com" target="_blank">chris.hamilton@ericsson.com</a>><br><b>Cc:</b> <a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a><br><b>Subject:</b> Re: [cfe-dev] Static check on memcpy()<u></u><u></u></p></div></div><p class="MsoNormal"><u></u> <u></u></p><div><div><p class="MsoNormal">On Thu, 3 Dec 2020 at 14:34, Chris Hamilton via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>> wrote:<u></u><u></u></p></div><div><blockquote style="border-top:none;border-right:none;border-bottom:none;border-left:1pt solid rgb(204,204,204);padding:0in 0in 0in 6pt;margin-left:4.8pt;margin-right:0in"><div><div><p class="MsoNormal">Hi folks,<u></u><u></u></p><p class="MsoNormal"> <u></u><u></u></p><p class="MsoNormal">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:<u></u><u></u></p><p class="MsoNormal"> <u></u><u></u></p><p style="margin:0in 0in 0.0001pt">   #include <string.h><u></u><u></u></p><p style="margin:0in 0in 0.0001pt">   struct S {<u></u><u></u></p><p style="margin:0in 0in 0.0001pt">      int x;<u></u><u></u></p><p style="margin:0in 0in 0.0001pt">      int xx;<u></u><u></u></p><p style="margin:0in 0in 0.0001pt">      int y[];<u></u><u></u></p><p style="margin:0in 0in 0.0001pt">   };<u></u><u></u></p><p style="margin:0in 0in 0.0001pt">   char dst[100];<u></u><u></u></p><p style="margin:0in 0in 0.0001pt">   <u></u><u></u></p><p style="margin:0in 0in 0.0001pt">   int main(void) {<u></u><u></u></p><p style="margin:0in 0in 0.0001pt">      struct S src = {0};<u></u><u></u></p><p style="margin:0in 0in 0.0001pt">      src.x = 9999;<u></u><u></u></p><p style="margin:0in 0in 0.0001pt">      src.xx = 8888;<u></u><u></u></p><p style="margin:0in 0in 0.0001pt">      memcpy(dst, &src, sizeof(struct S) + 1);<u></u><u></u></p><p style="margin:0in 0in 0.0001pt">   }<u></u><u></u></p><p class="MsoNormal"> <u></u><u></u></p><p class="MsoNormal">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?<u></u><u></u></p></div></div></blockquote><div><p class="MsoNormal"><u></u> <u></u></p></div><div><p class="MsoNormal">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.)<u></u><u></u></p></div><div><p class="MsoNormal"><u></u> <u></u></p></div><div><p class="MsoNormal">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.<u></u><u></u></p></div><div><p class="MsoNormal"> <u></u><u></u></p></div><blockquote style="border-top:none;border-right:none;border-bottom:none;border-left:1pt solid rgb(204,204,204);padding:0in 0in 0in 6pt;margin-left:4.8pt;margin-right:0in"><div><div><p class="MsoNormal">Regards,<u></u><u></u></p><p class="MsoNormal"><span style="font-size:8pt;font-family:Arial,sans-serif;color:rgb(24,24,24)"> </span><u></u><u></u></p><p class="MsoNormal"><a href="https://www.ericsson.com/" target="_blank"><span style="font-size:10pt;font-family:Arial,sans-serif;text-decoration:none"><img border="0" width="30" height="30" style="width: 0.3125in; height: 0.3125in;" id="gmail-m_6496434023715043067gmail-m_-2988126295748465746_x005f_x0000_i1026" src="https://mediabank.ericsson.net/internet-media/Email_logo_Ericsson.png"></span></a><u></u><u></u></p><p class="MsoNormal"><span style="font-size:10pt;font-family:Arial,sans-serif;color:rgb(24,24,24)"> </span><u></u><u></u></p><p class="MsoNormal"><b><span style="font-size:10pt;font-family:Arial,sans-serif;color:rgb(24,24,24)">Chris Hamilton</span></b><span style="font-size:10pt;font-family:Arial,sans-serif;color:rgb(24,24,24)"> </span><u></u><u></u></p><p class="MsoNormal"><span style="font-size:10pt;font-family:Arial,sans-serif;color:rgb(24,24,24)">Compiler Developer</span><u></u><u></u></p><p class="MsoNormal"><span style="font-size:10pt;font-family:Arial,sans-serif;color:rgb(24,24,24)">BNEW DNEW 4G5G BI BBI 10</span><u></u><u></u></p><p class="MsoNormal"><span style="font-size:10pt;font-family:Arial,sans-serif;color:rgb(24,24,24)">Mobile: +1-512-955-0143</span><u></u><u></u></p><p class="MsoNormal"><span style="font-size:10pt;font-family:Arial,sans-serif;color:rgb(24,24,24)"><a href="mailto:chris.hamilton@ericsson.com" target="_blank">chris.hamilton@ericsson.com</a></span><u></u><u></u></p><p class="MsoNormal"><span style="font-size:10pt;font-family:Arial,sans-serif;color:rgb(24,24,24)"> </span><u></u><u></u></p><p class="MsoNormal"><b><span style="font-size:10pt;font-family:Arial,sans-serif;color:rgb(24,24,24)">“Without inclusion, diversity is only a statistic.” </span></b><span style="font-size:10pt;font-family:Arial,sans-serif;color:rgb(24,24,24)"> <i>-- Börje Ekholm, CEO of Ericsson</i></span><u></u><u></u></p><p class="MsoNormal"><span style="font-size:10pt;font-family:Arial,sans-serif;color:rgb(24,24,24)"> </span><u></u><u></u></p><p class="MsoNormal"><span style="font-size:10pt;font-family:Arial,sans-serif;color:rgb(24,24,24)">Ericsson</span><u></u><u></u></p><p class="MsoNormal"><span style="font-size:10pt;font-family:Arial,sans-serif;color:rgb(24,24,24)">1703 W. 5th Street Suite 600</span><u></u><u></u></p><p class="MsoNormal"><span style="font-size:10pt;font-family:Arial,sans-serif;color:rgb(24,24,24)">78703,Austin, Texas</span><u></u><u></u></p><p class="MsoNormal"><span style="font-size:10pt;font-family:Arial,sans-serif;color:rgb(24,24,24)">United States</span><u></u><u></u></p><p class="MsoNormal"><span style="font-size:10pt;font-family:Arial,sans-serif;color:rgb(24,24,24)"><a href="https://www.ericsson.com/" target="_blank">ericsson.com</a></span><u></u><u></u></p><p class="MsoNormal"><span style="font-size:8pt;font-family:Arial,sans-serif;color:rgb(24,24,24)"> </span><u></u><u></u></p><p class="MsoNormal"><a href="https://www.ericsson.com/current_campaign" target="_blank"><span style="font-size:8pt;font-family:Arial,sans-serif;text-decoration:none"><img border="0" width="500" height="80" style="width: 5.2083in; height: 0.8333in;" id="gmail-m_6496434023715043067gmail-m_-2988126295748465746_x005f_x0000_i1025" src="https://mediabank.ericsson.net/internet-media/Email_Message.gif"></span></a><u></u><u></u></p><p class="MsoNormal"><span style="font-size:8pt;font-family:Arial,sans-serif;color:rgb(24,24,24)"> </span><u></u><u></u></p><p class="MsoNormal"><span style="font-size:8pt;font-family:Arial,sans-serif;color:rgb(24,24,24)">Our commitment to <a href="https://www.ericsson.com/thecompany/sustainability-corporateresponsibility" title="https://www.ericsson.com/thecompany/sustainability-corporateresponsibility" target="_blank">Technology for Good</a> and <a href="https://www.ericsson.com/thecompany/diversity-inclusion" title="https://www.ericsson.com/thecompany/diversity-inclusion" target="_blank">Diversity and Inclusion</a> contributes to positive change.<br>Follow us on: <a href="https://www.facebook.com/ericsson" title="https://www.facebook.com/ericsson" target="_blank">Facebook</a> <a href="https://www.linkedin.com/company/ericsson" title="https://www.linkedin.com/company/ericsson" target="_blank">LinkedIn</a> <a href="https://twitter.com/Ericsson" title="https://twitter.com/Ericsson" target="_blank">Twitter</a><br><br>Legal entity:</span><span style="font-size:8pt;font-family:Arial,sans-serif;color:rgb(51,51,51)">ERICSSON AB</span><span style="font-size:8pt;font-family:Arial,sans-serif;color:rgb(24,24,24)"> registration number </span><span style="font-size:8pt;font-family:Arial,sans-serif;color:rgb(51,51,51)">556056-6258</span><span style="font-size:8pt;font-family:Arial,sans-serif;color:rgb(24,24,24)">, registered office in </span><span style="font-size:8pt;font-family:Arial,sans-serif;color:rgb(51,51,51)">Stockholm</span><span style="font-size:8pt;font-family:Arial,sans-serif;color:rgb(24,24,24)">.<br>This communication is confidential. Our email terms: <a href="https://www.ericsson.com/en/legal/privacy/email-disclaimer" title="https://www.ericsson.com/en/legal/privacy/email-disclaimer" target="_blank">www.ericsson.com/en/legal/privacy/email-disclaimer</a></span><u></u><u></u></p><p class="MsoNormal"> <u></u><u></u></p></div></div><p class="MsoNormal">_______________________________________________<br>cfe-dev mailing list<br><a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a><br><a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a><u></u><u></u></p></blockquote></div></div></div></div></div></blockquote></div></div>