Bug 210646

Summary: REGRESSION (r260112): createArchiveList() leaks malloc memory on early returns due to an error
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: WebCore Misc.Assignee: David Kilzer (:ddkilzer) <ddkilzer>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=176729
Bug Depends on: 210456    
Bug Blocks:    
Attachments:
Description Flags
Patch v1
none
Patch for landing none

Description David Kilzer (:ddkilzer) 2020-04-17 03:09:26 PDT 
createArchiveList() leaks malloc memory on early returns due to an error.

This was introduced recently by:

Bug 210456: dictionaryValueOfType() in WebCoreArgumentCodersMac.mm can be replaced with dynamic_cf_cast<>()
<https://webkit.org/b/210456>
<https://trac.webkit.org/r260112>

Found by clang static analyzer running in deep mode.
Comment 1 Radar WebKit Bug Importer 2020-04-17 03:09:50 PDT 
<rdar://problem/61928031>
Comment 2 David Kilzer (:ddkilzer) 2020-04-17 03:13:24 PDT 
Created attachment 396751 [details]
Patch v1
Comment 3 Darin Adler 2020-04-17 09:59:04 PDT 
Comment on attachment 396751 [details]
Patch v1

We should use smart pointers, not raw pointers, so it is harder to make mistakes like this. We have a MallocPtr template that we could use to work with straight malloc/free. There’s a little work needed since by default it works with fastMalloc/fastFree, but it might be worthwhile.
Comment 4 Darin Adler 2020-04-17 10:08:26 PDT 
Comment on attachment 396751 [details]
Patch v1

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

> Source/WebKit/Shared/mac/WebCoreArgumentCodersMac.mm:143
> -    if (!extractDictionaryValue(representation, CFSTR("protocolProperties"), protocolProperties))
> -        return false;
> -    if (!extractDictionaryValue(representation, CFSTR("expectedContentLength"), expectedContentLength))
> -        return false;
> -    if (!extractDictionaryValue(representation, CFSTR("mimeType"), mimeType))
> +    if (!extractDictionaryValue(representation, CFSTR("protocolProperties"), protocolProperties)
> +        || !extractDictionaryValue(representation, CFSTR("expectedContentLength"), expectedContentLength)
> +        || !extractDictionaryValue(representation, CFSTR("mimeType"), mimeType)) {
> +        free(*objects);
> +        *objects = nullptr;
> +        *objectCount = 0;
>          return false;
> +    }

Another fix would be to do this checking and extraction before calling malloc. No reason things have to be done in this order
Comment 5 David Kilzer (:ddkilzer) 2020-04-17 15:32:28 PDT 
Created attachment 396808 [details]
Patch for landing
Comment 6 EWS 2020-04-17 16:07:03 PDT 
Committed r260299: <https://trac.webkit.org/changeset/260299>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396808 [details].