This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
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 <law@redhat.com> 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.
Richard.
> >
> > 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