On Sun, Dec 01, 2024 at 05:29:13PM -0500, Jeff King wrote: > On Fri, Nov 29, 2024 at 02:13:21PM +0100, Patrick Steinhardt wrote: > > > when compiling with DEVELOPER=YesPlease, we explicitly disable the > > "-Wsign-compare" warning. This is mostly because our code base is full > > of cases where we don't bother at all whether something should be signed > > or unsigned, and enabling the warning would thus cause tons of warnings > > to pop up. > > > > Unfortunately, disabling this warning also masks real issues. There have > > been multiple CVEs in the Git project that would have been flagged by > > this warning (e.g. CVE-2022-39260, CVE-2022-41903 and several fixes in > > the vicinity of these CVEs). Furthermore, the final audit report by > > X41 D-Sec, who are the ones who have discovered some of the CVEs, hinted > > that it might be a good idea to become more strict in this context. > > Yeah, this is something I've wanted to do for a long time. Your subject > line got me all excited that it was done, so I was a little disappointed > to see that it's the start of a long transition. :) For most of the files it only requires a couple of trivial changes to get things working. But overall we have been quite lenient with how we intermix integer types, so there's simply too many warnings to address in a single step. > Still, I think it is good to start, and the way you've laid it out seems > pretty reasonable to me. > > Regarding those CVEs, I suspect that -Wconversion is at least as > interesting there, as it catches direct assignments that may truncate > (I think those two were a little more complex, but a common issue is > then using the truncated computation to allocate a too-small buffer). Yeah, -Wconversion is on my radar as another step. I hope it will go smoother when we have already addressed -Wsign-compare, as that require us to fix a bunch of the same causes for warnings. We could also try to combine this into a single step, but think it might be easier if we handle these transitions separately. > But we have to start somewhere, and this may be a more tractable place. > The patches themselves looked mostly good to me, though the one with the > casts raised my eyebrows a bit (I left further comments there). Thanks, will have a look. Patrick
