Bug 71921 – Remove use of strcpy in KURL

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[David Kilzer (:ddkilzer)]

Created attachment 114288 [details]
Patch

Reviewed by NOBODY (OOPS!).

* platform/KURL.cpp:
(WebCore::KURL::init): Replace strcat() with strncat().
---
 2 files changed, 14 insertions(+), 2 deletions(-)

[David Kilzer (:ddkilzer)]

Comment on attachment 114288 [details]
Patch

Oops...left a fprintf in there.

[WebKit Review Bot]

Attachment 114288 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Kilzer (:ddkilzer)]

Created attachment 114291 [details]
Patch v2

Removed fprintf statement.

[David Kilzer (:ddkilzer)]

Comment on attachment 114291 [details]
Patch v2

Oops.  Need to subtract 1 from third argument to make sure the string is null-terminated.

[David Kilzer (:ddkilzer)]

Comment on attachment 114291 [details]
Patch v2

Nevermind...that's checked by the ASSERT below.

[David Kilzer (:ddkilzer)]

(In reply to comment #4)
> (From update of attachment 114291 [details])
> Oops.  Need to subtract 1 from third argument to make sure the string is null-terminated.

Also, that won't cause the string to be NULL-terminated.  The strlen(s2) must be less than n for that to hold.

[Andreas Kling]

Comment on attachment 114291 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=114291&action=review

> Source/WebCore/ChangeLog:3
> +        <http://webkit.org/b/71921> Remove use of strcat in KURL

strcpy, not strcat!

And a personal style nit, I think it would look better on two lines:
Remove use of strcpy in KURL
<http://webkit.org/b/71921>

> Source/WebCore/platform/KURL.cpp:552
> -                strcpy(bufferPos, relStringPos);
> +                strncpy(bufferPos, relStringPos, parserBufferSize - (bufferPosStart - bufferPos));

This looks backwards, shouldn't it be (bufferPos - bufferPosStart)?

[David Kilzer (:ddkilzer)]

(In reply to comment #7)
> (From update of attachment 114291 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114291&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        <http://webkit.org/b/71921> Remove use of strcat in KURL
> 
> strcpy, not strcat!

Wow, it's too early.

> And a personal style nit, I think it would look better on two lines:
> Remove use of strcpy in KURL
> <http://webkit.org/b/71921>
> 
> > Source/WebCore/platform/KURL.cpp:552
> > -                strcpy(bufferPos, relStringPos);
> > +                strncpy(bufferPos, relStringPos, parserBufferSize - (bufferPosStart - bufferPos));
> 
> This looks backwards, shouldn't it be (bufferPos - bufferPosStart)?

Yes!

[Darin Adler]

Comment on attachment 114291 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=114291&action=review

> Source/WebCore/platform/KURL.cpp:485
> +                const size_t parserBufferSize = base.m_pathEnd + 1 + len + 1;

We don’t normally use const like this for local variables. If we did, then bufferPosStart would also be const: const char* bufferPosStart. I suggest just leaving out the const.

> Source/WebCore/platform/KURL.cpp:489
> +                const char* bufferPosStart = bufferPos;

I think bufferStart is the right name for this, not bufferPosStart.

[David Kilzer (:ddkilzer)]

(In reply to comment #9)
> (From update of attachment 114291 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114291&action=review
> 
> > Source/WebCore/platform/KURL.cpp:485
> > +                const size_t parserBufferSize = base.m_pathEnd + 1 + len + 1;
> 
> We don’t normally use const like this for local variables. If we did, then bufferPosStart would also be const: const char* bufferPosStart. I suggest just leaving out the const.

Note that bufferPosStart is also const.

Why do we normally leave out const?  It's a hint that this value should never be altered within the method, which is exactly what I want.

> > Source/WebCore/platform/KURL.cpp:489
> > +                const char* bufferPosStart = bufferPos;
> 
> I think bufferStart is the right name for this, not bufferPosStart.

Okay.

[David Kilzer (:ddkilzer)]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 114291 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=114291&action=review
> > 
> > > Source/WebCore/platform/KURL.cpp:485
> > > +                const size_t parserBufferSize = base.m_pathEnd + 1 + len + 1;
> > 
> > We don’t normally use const like this for local variables. If we did, then bufferPosStart would also be const: const char* bufferPosStart. I suggest just leaving out the const.
> 
> Why do we normally leave out const?  It's a hint that this value should never be altered within the method, which is exactly what I want.

Or to put it another way: If someone came along later and added code that changed parserBufferSize/bufferSize between the time it was declared and the time it was used in strncpy(), that should be an error.  Removing the const-ness from the local variable would eliminate this error condition.  The same goes for bufferPosStart/bufferStart, which is why it was also made const.

[David Kilzer (:ddkilzer)]

Created attachment 114622 [details]
Patch v3

[Antti Koivisto]

Comment on attachment 114622 [details]
Patch v3

r=me

[David Kilzer (:ddkilzer)]

Committed r99999: <http://trac.webkit.org/changeset/99999>

[David Kilzer (:ddkilzer)]

<rdar://problem/6138531>

[David Kilzer (:ddkilzer)]

Comment on attachment 114622 [details]
Patch v3

Clearing cq flag as this was landed manually.

[Darin Adler]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > (From update of attachment 114291 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=114291&action=review
> > > 
> > > > Source/WebCore/platform/KURL.cpp:485
> > > > +                const size_t parserBufferSize = base.m_pathEnd + 1 + len + 1;
> > > 
> > > We don’t normally use const like this for local variables. If we did, then bufferPosStart would also be const: const char* bufferPosStart. I suggest just leaving out the const.
> > 
> > Why do we normally leave out const?  It's a hint that this value should never be altered within the method, which is exactly what I want.
> 
> Or to put it another way: If someone came along later and added code that changed parserBufferSize/bufferSize between the time it was declared and the time it was used in strncpy(), that should be an error.  Removing the const-ness from the local variable would eliminate this error condition.  The same goes for bufferPosStart/bufferStart, which is why it was also made const.

bufferStart was not made const. The characters it points to are const, but not the pointer. To make the pointer const you would need to write:

    const char* const bufferStart = bufferPos;

The majority of local variables are intended not to be modified after initialized. Yet we in the past we did not choose to put const in the code in all those places. We can head in a new direction, but typically I prefer to do that after a discussion with WebKit as a group, not in a single patch without that discussion.

[David Kilzer (:ddkilzer)]

(In reply to comment #17)
> bufferStart was not made const. The characters it points to are const, but not the pointer. To make the pointer const you would need to write:
> 
>     const char* const bufferStart = bufferPos;

Sorry, I was thinking about C-const-ness instead of C++-const-ness when writing this.

> The majority of local variables are intended not to be modified after initialized. Yet we in the past we did not choose to put const in the code in all those places. We can head in a new direction, but typically I prefer to do that after a discussion with WebKit as a group, not in a single patch without that discussion.

Filed Bug 72387.

[Antti Koivisto]

(In reply to comment #17)
> The majority of local variables are intended not to be modified after initialized. Yet we in the past we did not choose to put const in the code in all those places. We can head in a new direction, but typically I prefer to do that after a discussion with WebKit as a group, not in a single patch without that discussion.

That sounds bit dogmatic. I think the test should be whether using const in a particular case clarifies the code or usefully protects against a class of bugs. Do we really need a project-wide direction for this?

[Darin Adler]

(In reply to comment #19)
> I think the test should be whether using const in a particular case clarifies the code or usefully protects against a class of bugs.

Sure, lets look at an example if you think there is a case like that. I don’t think there are any. I’ve never seen a const local variable protect against a class of bugs. Or to put it another way, I’ve never fixed a bug and thought “that never would have happened if this local variable was const”.

> Do we really need a project-wide direction for this?

I think we do. It’s confusing to see const in one case and not see it in another nearly identical case while looking at multiple functions doing similar things. It’s similar to other seemingly trivial style decisions in that consistency makes it easier to concentrate on meaningful differences in the code.

[Antti Koivisto]

(In reply to comment #20)
> (In reply to comment #19)
> > I think the test should be whether using const in a particular case clarifies the code or usefully protects against a class of bugs.
> 
> Sure, lets look at an example if you think there is a case like that. I don’t think there are any. I’ve never seen a const local variable protect against a class of bugs. Or to put it another way, I’ve never fixed a bug and thought “that never would have happened if this local variable was const”.

I think any code that uses more than a few variables of the same type would benefit from marking the ones that are logically constant (start/end pointers and similar) as const to document the intentions.

For example the code discussed in this bug would benefit from making bufferStart, baseString and possibly some others const. Currently it is quite non-obvious which of the many variables the code is actually supposed to be modifying. This is the "clarifies the code" case which I think is not uncommon.

It will also make the compiler warn if you violate these intentions (and at least make you think if they need to be changed). For example modifying bufferStart/bufferSize over the course of this algorithm would be a bug.

If the code in question calls functions that return values in arguments accidental modification may be particularly hard to see. I didn't search for a good example in WebKit, don't know if it exist.

[Darin Adler]

(In reply to comment #21)
> I think any code that uses more than a few variables of the same type would benefit from marking the ones that are logically constant (start/end pointers and similar) as const to document the intentions.

OK.

> It will also make the compiler warn if you violate these intentions (and at least make you think if they need to be changed). For example modifying bufferStart/bufferSize over the course of this algorithm would be a bug.

I don’t think this has happened in practice, although I could be wrong.

> If the code in question calls functions that return values in arguments accidental modification may be particularly hard to see. I didn't search for a good example in WebKit, don't know if it exist.

Pretty sure it does not.

[Antti Koivisto]

(In reply to comment #22)
> I don’t think this has happened in practice, although I could be wrong.

Personally I make errors of all sorts as I write code, occasionally I think from this class too. Testing generally reveals them well before they land to WebKit in any case, but using tools like const might catch some of them early and so speed up the process. It seems strange to require removing them before landing as there is obvious harm (and there is the possible documentation benefit).

[Antti Koivisto]

I meant "no obvious harm"