Bug 160850 - [Win] Hardening of getLinkedFonts function.
Summary: [Win] Hardening of getLinkedFonts function.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-08-15 07:35 PDT by Per Arne Vollan
Modified: 2016-08-16 02:48 PDT (History)
5 users (show)

See Also:


Attachments
Patch (1.92 KB, patch)
2016-08-15 08:03 PDT, Per Arne Vollan
bfulgham: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2016-08-15 07:35:42 PDT
The SUCCEEDED macro should only be used for functions returning a HRESULT type.
Also, make sure a string array index will not exceed the string length.
Comment 1 Per Arne Vollan 2016-08-15 08:03:05 PDT
Created attachment 286053 [details]
Patch
Comment 2 Brent Fulgham 2016-08-15 08:43:02 PDT
Comment on attachment 286053 [details]
Patch

Interesting! I didn't know that SUCCEEDED was inappropriate here. Are there return values from RegQueryValueEx that might fool SUCCEEDED?

It also makes me wonder if we should be using the FAILED macro a few lines above this one, since it also returns a LONG instead of an HRESULT.

I also wonder if we should call "family.charactersWithNullTermination().data()" so many times -- I think (but didn't look) that it creates a new string each time we call that.

r=me! :-)
Comment 3 Per Arne Vollan 2016-08-15 09:39:20 PDT
(In reply to comment #2)
> Comment on attachment 286053 [details]
> Patch

Thanks for reviewing :-)

> 
> Interesting! I didn't know that SUCCEEDED was inappropriate here. Are there
> return values from RegQueryValueEx that might fool SUCCEEDED?
> 

Yes I believe there are.

> It also makes me wonder if we should be using the FAILED macro a few lines
> above this one, since it also returns a LONG instead of an HRESULT.
> 
> I also wonder if we should call
> "family.charactersWithNullTermination().data()" so many times -- I think
> (but didn't look) that it creates a new string each time we call that.
> 

Yes I believe you're right. I can look into that :-)
Comment 4 Per Arne Vollan 2016-08-15 09:41:22 PDT
rdar://problem/17471209
Comment 5 WebKit Commit Bot 2016-08-15 09:45:52 PDT
Comment on attachment 286053 [details]
Patch

Rejecting attachment 286053 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 286053, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.webkit.org/results/1875986
Comment 6 Myles C. Maxfield 2016-08-15 13:58:18 PDT
Please write a test for the comma change.
Comment 7 Per Arne Vollan 2016-08-16 02:48:56 PDT
Committed r204502: <https://trac.webkit.org/changeset/204502>