210646 2020-04-17 03:09:26 -0700 REGRESSION (r260112): createArchiveList() leaks malloc memory on early returns due to an error 2020-04-17 16:07:04 -0700 1 1 1 Unclassified WebKit WebCore Misc. WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=176729 InRadar P2 Normal --- 210456 1 ddkilzer ddkilzer darin webkit-bug-importer oldest_to_newest 1642621 0 ddkilzer 2020-04-17 03:09:26 -0700 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. 1642622 1 webkit-bug-importer 2020-04-17 03:09:50 -0700 <rdar://problem/61928031> 1642623 2 396751 ddkilzer 2020-04-17 03:13:24 -0700 Created attachment 396751 Patch v1 1642716 3 396751 darin 2020-04-17 09:59:04 -0700 Comment on attachment 396751 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. 1642722 4 396751 darin 2020-04-17 10:08:26 -0700 Comment on attachment 396751 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 1642870 5 396808 ddkilzer 2020-04-17 15:32:28 -0700 Created attachment 396808 Patch for landing 1642892 6 ews-feeder 2020-04-17 16:07:03 -0700 Committed r260299: <https://trac.webkit.org/changeset/260299> All reviewed patches have been landed. Closing bug and clearing flags on attachment 396808. 396751 2020-04-17 03:13:24 -0700 2020-04-17 15:32:26 -0700 Patch v1 bug-210646-20200417031439.patch text/plain 2127 ddkilzer U3VidmVyc2lvbiBSZXZpc2lvbjogMjYwMjI5CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IDM3NjFlZWE0NDNmN2NhYTJl N2U2OWFlZGQ1MjhkYjdmNWEwNTFlNWQuLmU3MTU0NDM1OTgwZDJlYWQ1ZmNhZGQxZTkxY2FkYjAz OTBhZmFiYjMgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTYgQEAKKzIwMjAtMDQtMTcgIERhdmlkIEtp bHplciAgPGRka2lsemVyQGFwcGxlLmNvbT4KKworICAgICAgICBCdWcgMjEwNjQ2OiBSRUdSRVNT SU9OIChyMjYwMTEyKTogY3JlYXRlQXJjaGl2ZUxpc3QoKSBsZWFrcyBtYWxsb2MgbWVtb3J5IG9u IGVhcmx5IHJldHVybnMgZHVlIHRvIGFuIGVycm9yCisgICAgICAgIDxodHRwczovL3dlYmtpdC5v cmcvYi8yMTA2NDY+CisgICAgICAgIDxyZGFyOi8vcHJvYmxlbS82MTkyODAzMT4KKworICAgICAg ICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIFNoYXJlZC9tYWMvV2Vi Q29yZUFyZ3VtZW50Q29kZXJzTWFjLm1tOgorICAgICAgICAoSVBDOjpjcmVhdGVBcmNoaXZlTGlz dCk6CisgICAgICAgIC0gRnJlZSBtYWxsb2MgbWVtb3J5IGFuZCByZXNldCBgb2JqZWN0c2AgYW5k IGBvYmplY3RDb3VudGAgb24KKyAgICAgICAgICBlYXJseSByZXR1cm5zIGR1ZSB0byBhIGRlY29k aW5nIGVycm9yLgorCiAyMDIwLTA0LTE2ICBEYXZpZCBLaWx6ZXIgIDxkZGtpbHplckBhcHBsZS5j b20+CiAKICAgICAgICAgUmUtbGFuZDogW0lQQyBIYXJkZW5pbmddIE1hY2hNZXNzYWdlOjpjcmVh dGUoKSBzaG91bGQgdXNlIGNoZWNrZWQgYXJpdGhtZXRpYwpkaWZmIC0tZ2l0IGEvU291cmNlL1dl YktpdC9TaGFyZWQvbWFjL1dlYkNvcmVBcmd1bWVudENvZGVyc01hYy5tbSBiL1NvdXJjZS9XZWJL aXQvU2hhcmVkL21hYy9XZWJDb3JlQXJndW1lbnRDb2RlcnNNYWMubW0KaW5kZXggMWU4OGU1ZDE4 NGQzMDQ1NzY0YWY2OTVjYzM3OTJiNGRkNzI2MDQ5OC4uOWQ4NzljODBiNzM2ZWJhMGFlYTMzYzFl OTc2MWY5ODdhZDQ5ODU5MiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdC9TaGFyZWQvbWFjL1dl YkNvcmVBcmd1bWVudENvZGVyc01hYy5tbQorKysgYi9Tb3VyY2UvV2ViS2l0L1NoYXJlZC9tYWMv V2ViQ29yZUFyZ3VtZW50Q29kZXJzTWFjLm1tCkBAIC0xMzMsMTIgKzEzMywxNCBAQCBzdGF0aWMg Ym9vbCBjcmVhdGVBcmNoaXZlTGlzdChDRkRpY3Rpb25hcnlSZWYgcmVwcmVzZW50YXRpb24sIENG VHlwZVJlZiB0b2tlbk51bAogICAgICAgICAgICAgKCpvYmplY3RzKVtpXSA9IG51bGxwdHI7CiAg ICAgfQogCi0gICAgaWYgKCFleHRyYWN0RGljdGlvbmFyeVZhbHVlKHJlcHJlc2VudGF0aW9uLCBD RlNUUigicHJvdG9jb2xQcm9wZXJ0aWVzIiksIHByb3RvY29sUHJvcGVydGllcykpCi0gICAgICAg IHJldHVybiBmYWxzZTsKLSAgICBpZiAoIWV4dHJhY3REaWN0aW9uYXJ5VmFsdWUocmVwcmVzZW50 YXRpb24sIENGU1RSKCJleHBlY3RlZENvbnRlbnRMZW5ndGgiKSwgZXhwZWN0ZWRDb250ZW50TGVu Z3RoKSkKLSAgICAgICAgcmV0dXJuIGZhbHNlOwotICAgIGlmICghZXh0cmFjdERpY3Rpb25hcnlW YWx1ZShyZXByZXNlbnRhdGlvbiwgQ0ZTVFIoIm1pbWVUeXBlIiksIG1pbWVUeXBlKSkKKyAgICBp ZiAoIWV4dHJhY3REaWN0aW9uYXJ5VmFsdWUocmVwcmVzZW50YXRpb24sIENGU1RSKCJwcm90b2Nv bFByb3BlcnRpZXMiKSwgcHJvdG9jb2xQcm9wZXJ0aWVzKQorICAgICAgICB8fCAhZXh0cmFjdERp Y3Rpb25hcnlWYWx1ZShyZXByZXNlbnRhdGlvbiwgQ0ZTVFIoImV4cGVjdGVkQ29udGVudExlbmd0 aCIpLCBleHBlY3RlZENvbnRlbnRMZW5ndGgpCisgICAgICAgIHx8ICFleHRyYWN0RGljdGlvbmFy eVZhbHVlKHJlcHJlc2VudGF0aW9uLCBDRlNUUigibWltZVR5cGUiKSwgbWltZVR5cGUpKSB7Cisg ICAgICAgIGZyZWUoKm9iamVjdHMpOworICAgICAgICAqb2JqZWN0cyA9IG51bGxwdHI7CisgICAg ICAgICpvYmplY3RDb3VudCA9IDA7CiAgICAgICAgIHJldHVybiBmYWxzZTsKKyAgICB9CiAKICAg ICByZXR1cm4gdHJ1ZTsKIH0K 396808 2020-04-17 15:32:28 -0700 2020-04-17 16:07:04 -0700 Patch for landing bug-210646-20200417153346.patch text/plain 2389 ddkilzer U3VidmVyc2lvbiBSZXZpc2lvbjogMjYwMjkwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IDExZjZlNzRiMWNiYmU4MGI4 N2YyNTU4MTE5NzkxYjYyODc5NmJjOWMuLmNlYzUzMjBhMzZiNWU2MmU0MzVkM2IzNmRkZTA5Zjgz ODZkOGZiMzkgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTYgQEAKKzIwMjAtMDQtMTcgIERhdmlkIEtp bHplciAgPGRka2lsemVyQGFwcGxlLmNvbT4KKworICAgICAgICBCdWcgMjEwNjQ2OiBSRUdSRVNT SU9OIChyMjYwMTEyKTogY3JlYXRlQXJjaGl2ZUxpc3QoKSBsZWFrcyBtYWxsb2MgbWVtb3J5IG9u IGVhcmx5IHJldHVybnMgZHVlIHRvIGFuIGVycm9yCisgICAgICAgIDxodHRwczovL3dlYmtpdC5v cmcvYi8yMTA2NDY+CisgICAgICAgIDxyZGFyOi8vcHJvYmxlbS82MTkyODAzMT4KKworICAgICAg ICBSZXZpZXdlZCBieSBEYXJpbiBBZGxlci4KKworICAgICAgICAqIFNoYXJlZC9tYWMvV2ViQ29y ZUFyZ3VtZW50Q29kZXJzTWFjLm1tOgorICAgICAgICAoSVBDOjpjcmVhdGVBcmNoaXZlTGlzdCk6 CisgICAgICAgIC0gTW92ZSBlYXJseSByZXR1cm5zIGZvciBwcm90b2NvbFByb3BlcnRpZXMsIHBy b3RvY29sUHJvcGVydGllcworICAgICAgICAgIGFuZCBtaW1lVHlwZSBhYm92ZSBtZW1vcnkgYWxs b2NhdGlvbiB0byBmaXggdGhlIG1lbW9yeSBsZWFrLgorCiAyMDIwLTA0LTE3ICBEYXZpZCBLaWx6 ZXIgIDxkZGtpbHplckBhcHBsZS5jb20+CiAKICAgICAgICAgUkVHUkVTU0lPTiAocjIzNDEwNSk6 IFtpT1NdIFdLQ29sb3JCdXR0b24gbGVha3MgYSBVSUNvbG9yCmRpZmYgLS1naXQgYS9Tb3VyY2Uv V2ViS2l0L1NoYXJlZC9tYWMvV2ViQ29yZUFyZ3VtZW50Q29kZXJzTWFjLm1tIGIvU291cmNlL1dl YktpdC9TaGFyZWQvbWFjL1dlYkNvcmVBcmd1bWVudENvZGVyc01hYy5tbQppbmRleCAxZTg4ZTVk MTg0ZDMwNDU3NjRhZjY5NWNjMzc5MmI0ZGQ3MjYwNDk4Li5mZWQzNzU0ZWNkY2MwMzQ0YzkzYTgz ZmZhZGY2Yzk0YTFiYWQyNGYzIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0L1NoYXJlZC9tYWMv V2ViQ29yZUFyZ3VtZW50Q29kZXJzTWFjLm1tCisrKyBiL1NvdXJjZS9XZWJLaXQvU2hhcmVkL21h Yy9XZWJDb3JlQXJndW1lbnRDb2RlcnNNYWMubW0KQEAgLTEyNCw2ICsxMjQsMTMgQEAgc3RhdGlj IGJvb2wgY3JlYXRlQXJjaGl2ZUxpc3QoQ0ZEaWN0aW9uYXJ5UmVmIHJlcHJlc2VudGF0aW9uLCBD RlR5cGVSZWYgdG9rZW5OdWwKICAgICBpZiAoYnVmZmVyU2l6ZS5oYXNPdmVyZmxvd2VkKCkpCiAg ICAgICAgIHJldHVybiBmYWxzZTsKIAorICAgIGlmICghZXh0cmFjdERpY3Rpb25hcnlWYWx1ZShy ZXByZXNlbnRhdGlvbiwgQ0ZTVFIoInByb3RvY29sUHJvcGVydGllcyIpLCBwcm90b2NvbFByb3Bl cnRpZXMpKQorICAgICAgICByZXR1cm4gZmFsc2U7CisgICAgaWYgKCFleHRyYWN0RGljdGlvbmFy eVZhbHVlKHJlcHJlc2VudGF0aW9uLCBDRlNUUigiZXhwZWN0ZWRDb250ZW50TGVuZ3RoIiksIGV4 cGVjdGVkQ29udGVudExlbmd0aCkpCisgICAgICAgIHJldHVybiBmYWxzZTsKKyAgICBpZiAoIWV4 dHJhY3REaWN0aW9uYXJ5VmFsdWUocmVwcmVzZW50YXRpb24sIENGU1RSKCJtaW1lVHlwZSIpLCBt aW1lVHlwZSkpCisgICAgICAgIHJldHVybiBmYWxzZTsKKwogICAgICpvYmplY3RDb3VudCA9IGFy Y2hpdmVMaXN0QXJyYXlDb3VudDsKICAgICAqb2JqZWN0cyA9IHN0YXRpY19jYXN0PENGVHlwZVJl Zio+KG1hbGxvYyhidWZmZXJTaXplLnVuc2FmZUdldCgpKSk7CiAKQEAgLTEzMywxMyArMTQwLDYg QEAgc3RhdGljIGJvb2wgY3JlYXRlQXJjaGl2ZUxpc3QoQ0ZEaWN0aW9uYXJ5UmVmIHJlcHJlc2Vu dGF0aW9uLCBDRlR5cGVSZWYgdG9rZW5OdWwKICAgICAgICAgICAgICgqb2JqZWN0cylbaV0gPSBu dWxscHRyOwogICAgIH0KIAotICAgIGlmICghZXh0cmFjdERpY3Rpb25hcnlWYWx1ZShyZXByZXNl bnRhdGlvbiwgQ0ZTVFIoInByb3RvY29sUHJvcGVydGllcyIpLCBwcm90b2NvbFByb3BlcnRpZXMp KQotICAgICAgICByZXR1cm4gZmFsc2U7Ci0gICAgaWYgKCFleHRyYWN0RGljdGlvbmFyeVZhbHVl KHJlcHJlc2VudGF0aW9uLCBDRlNUUigiZXhwZWN0ZWRDb250ZW50TGVuZ3RoIiksIGV4cGVjdGVk Q29udGVudExlbmd0aCkpCi0gICAgICAgIHJldHVybiBmYWxzZTsKLSAgICBpZiAoIWV4dHJhY3RE aWN0aW9uYXJ5VmFsdWUocmVwcmVzZW50YXRpb24sIENGU1RSKCJtaW1lVHlwZSIpLCBtaW1lVHlw ZSkpCi0gICAgICAgIHJldHVybiBmYWxzZTsKLQogICAgIHJldHVybiB0cnVlOwogfQogCg==