This is the mail archive of the mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 0/6] improve handling of char arrays with missing nul (PR 86552, 86711, 86714)

On Wed, Aug 15, 2018 at 5:42 PM Jeff Law <> wrote:
> On 08/15/2018 08:47 AM, Martin Sebor wrote:
> > On 08/15/2018 12:02 AM, Jeff Law wrote:
> >> On 08/13/2018 03:23 PM, Martin Sebor wrote:
> >>> To make reviewing the changes easier I've split up the patch
> >>> into a series:
> >> [ ... ]
> >> I'm about done for the night and thus won't get into the series (and as
> >> you know Bernd has a competing patch in this space).  But I did want to
> >> chime in on two things...
> >>
> >>>
> >>> There are many more string functions where unterminated (constant
> >>> or otherwise) should be diagnosed.  I plan to continue to work on
> >>> those (with the constant ones first)  but I want to post this
> >>> updated patch for review now, mainly so that the wrong code bug
> >>> (PR 86711) can be resolved and the basic detection infrastructure
> >>> agreed on.
> >> Yes, I think we definitely want to focus on the wrong code bug first.
> >>
> >>>
> >>> An open question in my mind is what should GCC do with such calls
> >>> after issuing a warning: replace them with traps?  Fold them into
> >>> constants?  Or continue to pass them through to the corresponding
> >>> library functions?
> >> My personal preference is to turn them into traps.  I don't think we
> >> have to preserve the call itself in this case.   I think the sequencing
> >> is to insert the trap before the call point, split the block after the
> >> trap, remove the outgoing edges, let DCE clean up the rest.  At least I
> >> think that's the sequencing.
> >
> > That sounds fine to me.  It would be close in its effects to
> > what _FORTIFY_SOURCE does.
> The bad guys are exceedingly resourceful in how they exploit undefined
> behavior.  By trapping immediately they don't have any window to do
> anything nefarious.
> >
> > It would be helpful to get a broader consensus on this and start
> > adopting the same consistent solution in all contexts.  The question
> > has come up a few times, most recently also in PR 86519 (folding
> > memcmp(a, "a", 3)) where GCC ends up calling the library function.
> Yup.  We've got a mish-mash of strategies here.

Folding cannot easily make sth "regular" as memcmp a noreturn thing.
At least not all callers expect that to happen.  So what you'd need to
do is ensure GF_CALL_CTRL_ALTERING is not set on the replacement
trap().  The next fixup_cfg () pass will fix things for you then.

> >
> > FWIW, if there are other preferences it might be worthwhile to
> > consider providing an option to control the behavior in these
> > cases.  There may also be interactions with or implications for
> > the sanitizers to consider.
> There's some (Marc Glisse IIRC) that would prefer to see the control
> path to the undefined behavior zapped entirely.  We didn't initially do
> that because the path my have other observable side effects.  However,
> there may be cases where it makes sense.

You can't remove observable side-effects and given that there exist
things like signal handlers for SIGSEGV even changing a memcmp
to __builtin_trap() may change observable behavior.

This is why some places in GCC simply refuse to optimize "broken"
cases but keep calling the library.


> >
> > Once there is agreement on what the solution should be I can look
> > into implementing it at some point in the future.
> ACK.  Certainly lower priority than the stuff in flight right now.
> jeff

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]