From: Jeff Law <law@redhat.com>
To: Bernd Edlinger <bernd.edlinger@hotmail.de>,
Martin Sebor <msebor@gmail.com>,
Gcc Patch List <gcc-patches@gcc.gnu.org>
Subject: Re: PING [PATCH] warn for strlen of arrays with missing nul (PR 86552, 86711, 86714) )
Date: Sat, 25 Aug 2018 17:33:00 -0000 [thread overview]
Message-ID: <fd265127-aca6-6a05-991e-e16b172c42f6@redhat.com> (raw)
In-Reply-To: <AM5PR0701MB2657E36E54AB1B39064A58B3E4350@AM5PR0701MB2657.eurprd07.prod.outlook.com>
On 08/25/2018 12:32 AM, Bernd Edlinger wrote:
> On 08/25/18 01:54, Jeff Law wrote:
>> On 08/24/2018 11:26 AM, Bernd Edlinger wrote:
>>> On 08/24/18 18:51, Jeff Law wrote:
>>>>> Well, this is broken for wide character strings.
>>>>> but I hope we can get rid of STRING_CST which are
>>>>> not explicitly null terminated.
>>>
>>> I am afraid that is not going to happen.
>>> Maybe we can get STRING_CST that are never longer
>>> than the TYPE_UNIT_SIZE, but c_strlen and c_getstr
>>> need to take care that the string is zero-terminated.
>>>
>>> string_constant, should not promise the string is zero terminated.
>>> But instead it can promise that:
>>> 1) the STRING_CST is valid up to TREE_STRING_LENGTH
>>> 2) mem_size is >= TREE_STRING_LENGTH
>>> 3) memory between TREE_STRING_LENGTH and mem_size is ZERO.
>>>
>>> It will not guarantee anything about zero termination any more.
>> Interesting because those conditions would be sufficient to deal with a
>> regression I stumbled over after fixing Martin's patch to not assume
>> that all STRING_CSTs are NUL terminated.
>>
>> But I need to think about this a bit more. Essentially the question
>> we'd need to ask is whether or not these are sufficient in general or
>> just in specific cases.
>>
>> I tend to think they're not sufficient in general. If a string returned
>> by string_constant that didn't have a terminating NUL, but which did
>> pass the tests above were ultimately passed to the runtime's str*
>> routines, then the call may run off the end of the string. We'd like to
>> be able to warn for that.
>>
>> So ISTM those rules are only valid in contexts where we know the result
>> isn't going to be passed to str* and friends within the C library.
>>
>> I do think they're sufficient to avoid problems with the
>> tree-ssa-forwprop code we've looked at. So what may make the most sense
>> is to have that routine indicate it's willing to accept unterminated
>> strings, then check the conditions above before optimizing the code.
>>
>
> There are not too many callers of string_constant.
> Not all need zero termination.
Right. And in retrospect we probably should have avoided default
parameter overloads and just fixed the callers. But that can be a
follow-up.
>
> But I think if the are interested in zero-termination
> they should simply call c_strlen or c_getstr.
Perhaps.
>
>
>>>
>>> In the end, the best approach might be to either merge my patch
>>> with Martins, or step-wise, first fixing wrong code, and then
>>> implementing warnings without fixing wrong code.
>> Unsure at this time. I've been working with both. I suspect that if we
>> went with yours that we'd then turn around and layer Martin's on top of
>> it because of the desire to signal to callers that we have an
>> unterminated string and have the callers take appropriate action. Which
>> begs the question of whether or not we just go with Martin's -- ie, is
>> there really any value in using both. I haven't seen indications there
>> is value in that approach, but I'm still poking at things.
>>
>
> Well, ya call it "layer one patch over the other"
> I call it "incremental improvements".
It is (of course) a case by case basis. The way I try to look at these
things is to ask whether or not the first patch under consideration
would have any value/purpose after the second patch was installed. If
so, then it may make sense to include both. If not, then we really just
want one patch.
Jeff
next prev parent reply other threads:[~2018-08-25 17:33 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-07-19 20:09 [PATCH] warn for strlen of arrays with missing nul (PR 86552) Martin Sebor
2018-07-25 23:38 ` PING " Martin Sebor
2018-07-30 19:18 ` Martin Sebor
2018-08-02 2:44 ` PING [PATCH] warn for strlen of arrays with missing nul (PR 86552, 86711, 86714) ) Martin Sebor
2018-08-02 13:26 ` Bernd Edlinger
2018-08-02 18:56 ` Bernd Edlinger
2018-08-02 20:34 ` Martin Sebor
2018-08-03 13:01 ` Bernd Edlinger
2018-08-03 19:59 ` Martin Sebor
2018-08-15 5:31 ` Jeff Law
2018-08-29 17:17 ` Jeff Law
2018-08-24 6:36 ` Jeff Law
2018-08-24 12:28 ` Bernd Edlinger
2018-08-24 16:04 ` Jeff Law
2018-08-24 21:56 ` Bernd Edlinger
2018-08-24 16:51 ` Jeff Law
2018-08-24 17:26 ` Bernd Edlinger
2018-08-24 23:54 ` Jeff Law
2018-08-25 6:32 ` Bernd Edlinger
2018-08-25 17:33 ` Jeff Law [this message]
2018-08-25 18:36 ` Bernd Edlinger
2018-08-25 19:02 ` Jeff Law
2018-08-25 19:32 ` Bernd Edlinger
2018-08-25 20:42 ` Martin Sebor
2018-08-26 10:20 ` Bernd Edlinger
2018-08-25 23:22 ` Jeff Law
2018-08-17 5:15 ` Jeff Law
2018-08-17 14:38 ` Martin Sebor
2018-08-13 21:23 ` [PATCH 0/6] improve handling of char arrays with missing nul (PR 86552, 86711, 86714) Martin Sebor
2018-08-13 21:25 ` [PATCH 1/6] prevent folding of unterminated const arrays in memchr calls (PR " Martin Sebor
2018-08-13 21:27 ` [PATCH 3/6] detect unterminated const arrays in strcpy calls (PR 86552) Martin Sebor
2018-08-30 22:31 ` Jeff Law
2018-08-13 21:28 ` [PATCH 4/6] detect unterminated const arrays in sprintf " Martin Sebor
2018-08-30 22:55 ` Jeff Law
2018-08-13 21:29 ` [PATCH 6/6] detect unterminated const arrays in strnlen " Martin Sebor
2018-08-30 23:25 ` Jeff Law
2018-10-01 21:49 ` Jeff Law
2018-08-13 21:29 ` [PATCH 5/6] detect unterminated const arrays in stpcpy " Martin Sebor
2018-08-30 23:07 ` Jeff Law
2018-09-14 18:39 ` Jeff Law
2018-08-14 3:21 ` [PATCH 2/6] detect unterminated const arrays in strlen " Martin Sebor
2018-08-30 22:15 ` Jeff Law
2018-08-31 2:25 ` Martin Sebor
2018-08-15 6:02 ` [PATCH 0/6] improve handling of char arrays with missing nul (PR 86552, 86711, 86714) Jeff Law
2018-08-15 14:47 ` Martin Sebor
2018-08-15 15:42 ` Jeff Law
2018-08-24 10:13 ` Richard Biener
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=fd265127-aca6-6a05-991e-e16b172c42f6@redhat.com \
--to=law@redhat.com \
--cc=bernd.edlinger@hotmail.de \
--cc=gcc-patches@gcc.gnu.org \
--cc=msebor@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).