This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)
- From: Jeff Law <law at redhat dot com>
- To: Martin Sebor <msebor at gmail dot com>, Richard Biener <rguenther at suse dot de>, Gcc Patch List <gcc-patches at gcc dot gnu dot org>, Jakub Jelinek <jakub at redhat dot com>, Bernd Schmidt <bschmidt at redhat dot com>, David Malcolm <dmalcolm at redhat dot com>, Manuel López-Ibáñez <lopezibanez at gmail dot com>, Florian Weimer <fweimer at redhat dot com>, Joseph Myers <joseph at codesourcery dot com>
- Date: Fri, 19 Aug 2016 09:34:40 -0600
- Subject: Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)
- Authentication-results: sourceware.org; auth=none
- References: <5776B33E.2080504@gmail.com> <alpine.LSU.2.11.1607041255060.29772@t29.fhfr.qr> <577A8D6A.3070902@gmail.com> <alpine.LSU.2.11.1607051208470.29772@t29.fhfr.qr> <578D512F.9050909@gmail.com> <9bb5ad66-4985-8c42-f800-4d84e0e18659@redhat.com> <57A3AFFF.7090109@gmail.com> <f3d09350-de5d-a0e4-8203-affac268ced2@redhat.com> <57AD30E5.3000801@gmail.com> <22a47656-c23c-4840-2e49-a59f4af513b1@redhat.com> <57B725F6.8000405@gmail.com>
On 08/19/2016 09:29 AM, Martin Sebor wrote:
My biggest concern with this iteration is the tight integration between
the optimization and warning. We generally avoid that kind of tight
integration such that enabling the warning does not affect the
optimization and vice-versa.
So ISTM you have to do the analysis if the optimization or warning has
been requested. Then you conditionalize whether or not the warnings are
emitted by their flag and the optimization based on its flag.
As we discussed in IRC yesterday, the warning and the optimization
are independent of one another, and each controlled by its own option
(-Wformat-length and -fprintf-return-value). In light of that we've
agreed that submitting both as part of the same patch is sufficient.
Right. I must have mis-read something. I'll look for the tidbit that
made me think they were more intertwined than they really are. It may
be the case that we just want to tweak a comment.
I understand you're going to have some further work to do because of
conflicts with David's patches. With that in mind, I'd suggest a bit of
carving things up so things can start moving forward.
Patch #1. All the fixes to static buffer sizes that were inspired by
your warning. These are all approved and can go in immediately.
Attached is this patch.
Approved. Please install.
Patch #2. Improvement to __builtin_object_size to handle
POINTER_PLUS_EXPR on arrays. This is something that stands on it own
and ought to be reviewable quickly and doesn't really belong in the
bowels of the warning/optimization patch you're developing.
Sure. I'll submit this patch next.
Excellent.
Patch #5 and beyond: Further optimization work.
As one of the next steps I'd like to make this feature available
to user-defined sprintf-like functions decorated with attribute
format. To do that, I'm thinking of adding either a fourth
(optional) argument to attribute format printf indicating which
of the function arguments is the destination buffer (to compute
its size), or perhaps a new attribute under its own name. I'm
actually leaning toward latter since I think it could be used
in other contexts as well. I welcome comments and suggestions
on this idea.
Whichever we think will be easier to use and thus encourage folks to
annotate their code properly :-)
jeff