Bug 145608

Summary: Fix strncpy use in WebCore::Text::formatForDebugger
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Major CC: cdumez, cfleizach, commit-queue, darin, ddkilzer, esprehn+autocc, gyuyoung.kim, kangil.han, mcatanzaro
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
See Also: https://bugs.webkit.org/show_bug.cgi?id=145596
https://bugs.webkit.org/show_bug.cgi?id=145283
Attachments:
Description Flags
Patch
none
Patch none

Description Michael Catanzaro 2015-06-03 09:58:54 PDT
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.
Comment 1 Michael Catanzaro 2015-06-03 10:06:03 PDT
Created attachment 254184 [details]
Patch
Comment 2 Darin Adler 2015-06-03 14:30:16 PDT
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.
Comment 3 Darin Adler 2015-06-03 14:30:48 PDT
I see, we switched back from strlcpy to this. Please use strncpy the right way, then.
Comment 4 Michael Catanzaro 2015-06-04 08:15:29 PDT
(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/
Comment 5 Michael Catanzaro 2015-06-04 09:14:25 PDT
Created attachment 254266 [details]
Patch
Comment 6 Michael Catanzaro 2015-06-04 09:20:11 PDT
Is it typical to assign issues like these to the security product? I will do so, as a precaution.
Comment 7 WebKit Commit Bot 2015-06-04 11:25:17 PDT
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
Comment 8 Michael Catanzaro 2015-06-04 12:12:20 PDT
'webkit-patch land' is hanging for me, so I don't know how to land this. :)
Comment 9 Darin Adler 2015-06-05 14:46:13 PDT
Dave Kilzer, maybe you can help Michael get this patch landed?
Comment 10 Michael Catanzaro 2015-06-07 11:21:59 PDT
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.
Comment 11 Darin Adler 2015-06-07 17:50:29 PDT
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.
Comment 12 WebKit Commit Bot 2015-06-07 18:40:06 PDT
Comment on attachment 254266 [details]
Patch

Clearing flags on attachment: 254266

Committed r185309: <http://trac.webkit.org/changeset/185309>
Comment 13 WebKit Commit Bot 2015-06-07 18:40:09 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Lucas Forschler 2019-02-06 09:03:58 PST
Mass moving XML DOM bugs to the "DOM" Component.