Bug 210646 - REGRESSION (r260112): createArchiveList() leaks malloc memory on early returns due to an error
Summary: REGRESSION (r260112): createArchiveList() leaks malloc memory on early return...
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: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on: 210456
Blocks:
  Show dependency tree / graph
 
Reported: 2020-04-17 03:09 PDT by David Kilzer (:ddkilzer)
Modified: 2020-04-17 16:07 PDT (History)
2 users (show)

See Also:

Attachments
Patch v1 (2.08 KB, patch)
2020-04-17 03:13 PDT, David Kilzer (:ddkilzer) 		no flags Details | Formatted Diff | Diff
Patch for landing (2.33 KB, patch)
2020-04-17 15:32 PDT, David Kilzer (:ddkilzer) 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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].