Fix strncpy use in WebCore::Text::formatForDebugger

• Component: DOM
• Status: RESOLVED FIXED

[Michael Catanzaro]

r185137 replaced a call to strncpy with a call to strlcpy, which broke the build on Linux since strlcpy does not exist there. r185148 reverted this to use strncpy again, but got the size argument off by one, introducing a buffer overrun.

[Michael Catanzaro]

Created attachment 254184 [details]
Patch

[Darin Adler]

Comment on attachment 254184 [details]
Patch

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

> Source/WebCore/dom/Text.cpp:238
> -    strncpy(buffer, result.toString().utf8().data(), length);
> +    strncpy(buffer, result.toString().utf8().data(), length - 1);

This helpful but not a complete fix. As explained in man pages the world over, using strncpy for this purpose requires:

    strncpy(buffer, input, length - 1);
    buffer[length - 1] = '\0';

We normally prefer strlcpy for that reason, but not sure if strlcpy is available on all the platforms we care about.

[Darin Adler]

I see, we switched back from strlcpy to this. Please use strncpy the right way, then.

[Michael Catanzaro]

(In reply to comment #2)
> This helpful but not a complete fix.

Thanks for catching this; I will land with that fixed.

(In reply to comment #2)
> We normally prefer strlcpy for that reason, but not sure if strlcpy is
> available on all the platforms we care about.

Here's an article about why strlcpy is not available on Linux: https://lwn.net/Articles/612244/

[Michael Catanzaro]

Created attachment 254266 [details]
Patch

[Michael Catanzaro]

Is it typical to assign issues like these to the security product? I will do so, as a precaution.

[WebKit Commit Bot]

Comment on attachment 254266 [details]
Patch

Rejecting attachment 254266 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 254266, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
nts_and_execute
    return self.execute(options, args, tool) or 0
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/commands/download.py", line 173, in execute
    bugs_to_patches = self._collect_patches_by_bug(patches)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/commands/download.py", line 165, in _collect_patches_by_bug
    bugs_to_patches[patch.bug_id()] = bugs_to_patches.get(patch.bug_id(), []) + [patch]
AttributeError: 'NoneType' object has no attribute 'bug_id'

Full output: http://webkit-queues.appspot.com/results/6374772183138304

[Michael Catanzaro]

'webkit-patch land' is hanging for me, so I don't know how to land this. :)

[Darin Adler]

Dave Kilzer, maybe you can help Michael get this patch landed?

[Michael Catanzaro]

I also tried 'webkit-patch land-cowhand' which also hung. webkit-patch used to work for me. Due to time commitments on another project, I won't debug further. I don't mind much, as I normally just use the cq+ Bugzilla flag, but I guess that maybe doesn't work for security product bugs.

[Darin Adler]

While it’s OK to treat this the way we treat some security bugs since it does involve a buffer overrun in theory, I don’t think it’s critical to have it inside the hidden component.

[WebKit Commit Bot]

Comment on attachment 254266 [details]
Patch

Clearing flags on attachment: 254266

Committed r185309: <http://trac.webkit.org/changeset/185309>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Lucas Forschler]

Mass moving XML DOM bugs to the "DOM" Component.