Bug 160850

Summary: [Win] Hardening of getLinkedFonts function.
Product: WebKit Reporter: Per Arne Vollan <pvollan>
Component: WebCore Misc.Assignee: Per Arne Vollan <pvollan>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham, commit-queue, mmaxfield, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch bfulgham: review+, commit-queue: commit-queue-

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>