19287 2008-05-28 01:58:53 -0700 return value of malloc() is not checked in npruntime.cpp 2009-01-11 02:27:17 -0800 1 1 1 Unclassified WebKit Plug-ins 528+ (Nightly build) PC All RESOLVED FIXED P2 Normal --- 1 pepe webkit-unassigned andersca ap darin oldest_to_newest 81702 0 pepe 2008-05-28 01:58:53 -0700 The return values of the following malloc invocations are not checked (rev. 34169): WebKit/WebCore/bridge/npruntime.cpp(106): 106: identifier = (PrivateIdentifier*)malloc(sizeof(PrivateIdentifier)); 107: identifier->isString = false; 108: identifier->value.number = intid; WebKit/WebCore/bridge/npruntime.cpp(115): 115: identifier = (PrivateIdentifier*)malloc(sizeof(PrivateIdentifier)); 116: // We never release identifier names, so this dictionary will grow. 117: identifier->isString = false; 118: identifier->value.number = intid; WebKit/WebCore/bridge/npruntime.cpp(153) 153: variant->value.stringValue.UTF8Characters = (NPUTF8 *)malloc(sizeof(NPUTF8) * value->UTF8Length); 154: memcpy((void*)variant->value.stringValue.UTF8Characters, value->UTF8Characters, sizeof(NPUTF8) * value->UTF8Length); 81712 1 ap 2008-05-28 05:31:03 -0700 While technically true, I'm not sure if we want to track this as a bug - this is a small allocation, so out of memory condition is unlikely to be ever hit in practice. Even if this happens, the application will just crash, without letting anyone write to random memory locations. Aborting is what we'd likely have to do in an explicit check anyway. See also: bug 18670. 81735 2 eric 2008-05-28 08:45:22 -0700 I'd prefer this is duped against the "malloc should crash instead of ever returning null" bug. But I'm not sure all of the other webkit committers are in agreement w/ me on that one. :) Confirming that not-checking a null-returning malloc is bad, yes. :) 81739 3 darin 2008-05-28 08:59:04 -0700 I suggest we call new or fastMalloc here as we do in the rest of our code. I can't see any reason to use the system malloc. 100828 4 gtoth 2008-12-01 06:56:22 -0800 In wtf/Fastmalloc.cpp, the allocation is checked therefore we should check this here as well. We can use the same method here: if the allocation is unsuccessful, CRASH macro can be invoked. 100829 5 25630 gtoth 2008-12-01 06:57:20 -0800 Created attachment 25630 proposed patch 100837 6 25630 andersca 2008-12-01 10:06:48 -0800 Comment on attachment 25630 proposed patch I like Darin's suggestion better, if we use fastMalloc instead of malloc it should take care of crashing if the allocation fails. 100984 7 gtoth 2008-12-01 23:56:21 -0800 But according to another bug thread (bug #20566, comment #1) we should really call NPN_MemAlloc but it doesn't work on Mac. Because of this thread I didn't use fastmalloc here but I try to use its same functionality (CRASH). Should fastmalloc be used here as well? 102556 8 darin 2008-12-15 06:20:39 -0800 (In reply to comment #7) > But according to another bug thread (bug #20566, comment #1) we should really > call NPN_MemAlloc but it doesn't work on Mac. That NPN_MemAlloc comment in NP_jsobject.cpp is long-obsolete. That comment was written back when this code was in JavaScriptCore and NPN_MemAlloc was in WebKit. But now both the code and the NPN_MemAlloc function are in WebCore. So that issues is not an obstacle to using fastMalloc and fastFree here. On the other hand, there might be a risk with plug-in compatibility, if plug-ins themselves accidentally mix malloc/free with NPN_MemAlloc and NPN_MemFree. 102557 9 darin 2008-12-15 06:43:25 -0800 We want to avoid having to sprinkle CRASH calls all around the project, so if we can use fastMalloc or new that would be considerably more elegant. For PrivateIdentifier, new would be just fine. There's no need to call malloc there. For UTF8Characters I think we're stuck with malloc because, for example, I see code in testbindings.cpp setting UTF8Characters by calling strdup. I don't think we're free to use a malloc other than the system malloc in that case. There are also invocations of strdup, and the issue of failure handling is no different for strdup than for malloc. I think I see at least one case where someone expects to use strdup to allocate and then the caller will free with either NPN_Free or free, so I think we can't change those to fastMalloc. 102558 10 darin 2008-12-15 07:07:00 -0800 (In reply to comment #8) > That NPN_MemAlloc comment in NP_jsobject.cpp is long-obsolete. That comment was > written back when this code was in JavaScriptCore and NPN_MemAlloc was in > WebKit. But now both the code and the NPN_MemAlloc function are in WebCore. So > that issues is not an obstacle to using fastMalloc and fastFree here. I was wrong about that. The NPN_MemAlloc that is in WebCore is for non-Mac platforms. It's still true on the Mac. 102570 11 andersca 2008-12-15 10:52:53 -0800 (In reply to comment #10) > > I was wrong about that. The NPN_MemAlloc that is in WebCore is for non-Mac > platforms. It's still true on the Mac. > We can still use FastMalloc from WebKit though, right? 102572 12 darin 2008-12-15 11:18:29 -0800 (In reply to comment #11) > We can still use FastMalloc from WebKit though, right? Yes. But as I said earlier "there might be a risk with plug-in compatibility, if plug-ins themselves accidentally mix malloc/free with NPN_MemAlloc and NPN_MemFree." 104114 13 25630 darin 2009-01-02 10:53:36 -0800 Comment on attachment 25630 proposed patch Seems OK to land this as-is. I don't see a lot of harm in it despite the fact that we'd prefer to use fastMalloc. 105198 14 ap 2009-01-11 02:27:17 -0800 Committed revision 39792. 25630 2008-12-01 06:57:20 -0800 2009-01-02 10:53:36 -0800 proposed patch patch.txt text/plain 3053 gtoth SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiAzODg2MCkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTcgQEAKKzIwMDgtMTItMDEgIEdhYnJpZWxsYSBUb3RoICA8Z3RvdGhAaW5mLnUt c3plZ2VkLmh1PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgIENoZWNraW5nIHdoZXRoZXIgbWFsbG9jIGNhbiBhbGxvY2F0ZSBtZW1vcnkgb3Igbm90LiBJ ZiBpdCBjYW4ndCwgQ1JBU0ggbWFjcm8gaXMgaW52b2tlZChsaWtlIGluIGZhc3RtYWxsb2MpLgor ICAgCisgICAgICAgIFdBUk5JTkc6IE5PIFRFU1QgQ0FTRVMgQURERUQgT1IgQ0hBTkdFRAorCisg ICAgICAgICogYnJpZGdlL25wcnVudGltZS5jcHA6CisgICAgICAgIChfTlBOX0dldFN0cmluZ0lk ZW50aWZpZXIpOgorICAgICAgICAoX05QTl9HZXRJbnRJZGVudGlmaWVyKToKKyAgICAgICAgKE5Q Tl9Jbml0aWFsaXplVmFyaWFudFdpdGhTdHJpbmdDb3B5KToKKyAgICAgICAgKF9OUE5fQ3JlYXRl T2JqZWN0KToKKwogMjAwOC0xMi0wMSAgVG9yIEFybmUgVmVzdGLDuCAgPHRhdmVzdGJvQHRyb2xs dGVjaC5jb20+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgU2ltb24gSGF1c21hbm4uCkluZGV4OiBX ZWJDb3JlL2JyaWRnZS9ucHJ1bnRpbWUuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvYnJpZGdl L25wcnVudGltZS5jcHAJKHJldmlzaW9uIDM4ODU5KQorKysgV2ViQ29yZS9icmlkZ2UvbnBydW50 aW1lLmNwcAkod29ya2luZyBjb3B5KQpAQCAtNzEsNiArNzEsOCBAQCBOUElkZW50aWZpZXIgX05Q Tl9HZXRTdHJpbmdJZGVudGlmaWVyKGNvCiAgICAgICAgIGlkZW50aWZpZXIgPSBnZXRTdHJpbmdJ ZGVudGlmaWVyTWFwKCktPmdldChpZGVudGlmaWVyRnJvbU5QSWRlbnRpZmllcihuYW1lKS51c3Ry aW5nKCkucmVwKCkpOwogICAgICAgICBpZiAoaWRlbnRpZmllciA9PSAwKSB7CiAgICAgICAgICAg ICBpZGVudGlmaWVyID0gKFByaXZhdGVJZGVudGlmaWVyKiltYWxsb2Moc2l6ZW9mKFByaXZhdGVJ ZGVudGlmaWVyKSk7CisgICAgICAgICAgICBpZiAoIWlkZW50aWZpZXIpCisgICAgICAgICAgICAg ICAgQ1JBU0goKTsKICAgICAgICAgICAgIC8vIFdlIG5ldmVyIHJlbGVhc2UgaWRlbnRpZmllciBu YW1lcywgc28gdGhpcyBkaWN0aW9uYXJ5IHdpbGwgZ3JvdywgYXMgd2lsbAogICAgICAgICAgICAg Ly8gdGhlIG1lbW9yeSBmb3IgdGhlIGlkZW50aWZpZXIgbmFtZSBzdHJpbmdzLgogICAgICAgICAg ICAgaWRlbnRpZmllci0+aXNTdHJpbmcgPSB0cnVlOwpAQCAtMTA0LDYgKzEwNiw4IEBAIE5QSWRl bnRpZmllciBfTlBOX0dldEludElkZW50aWZpZXIoaW50MzIKICAgICAgICAgaWRlbnRpZmllciA9 IG5lZ2F0aXZlT25lQW5kWmVyb0lkZW50aWZpZXJzW2ludGlkICsgMV07CiAgICAgICAgIGlmICgh aWRlbnRpZmllcikgewogICAgICAgICAgICAgaWRlbnRpZmllciA9IChQcml2YXRlSWRlbnRpZmll ciopbWFsbG9jKHNpemVvZihQcml2YXRlSWRlbnRpZmllcikpOworICAgICAgICAgICAgaWYgKCFp ZGVudGlmaWVyKQorICAgICAgICAgICAgICAgIENSQVNIKCk7CiAgICAgICAgICAgICBpZGVudGlm aWVyLT5pc1N0cmluZyA9IGZhbHNlOwogICAgICAgICAgICAgaWRlbnRpZmllci0+dmFsdWUubnVt YmVyID0gaW50aWQ7CiAKQEAgLTExMyw2ICsxMTcsOCBAQCBOUElkZW50aWZpZXIgX05QTl9HZXRJ bnRJZGVudGlmaWVyKGludDMyCiAgICAgICAgIGlkZW50aWZpZXIgPSBnZXRJbnRJZGVudGlmaWVy TWFwKCktPmdldChpbnRpZCk7CiAgICAgICAgIGlmICghaWRlbnRpZmllcikgewogICAgICAgICAg ICAgaWRlbnRpZmllciA9IChQcml2YXRlSWRlbnRpZmllciopbWFsbG9jKHNpemVvZihQcml2YXRl SWRlbnRpZmllcikpOworICAgICAgICAgICAgaWYgKCFpZGVudGlmaWVyKQorICAgICAgICAgICAg ICAgIENSQVNIKCk7CiAgICAgICAgICAgICAvLyBXZSBuZXZlciByZWxlYXNlIGlkZW50aWZpZXIg bmFtZXMsIHNvIHRoaXMgZGljdGlvbmFyeSB3aWxsIGdyb3cuCiAgICAgICAgICAgICBpZGVudGlm aWVyLT5pc1N0cmluZyA9IGZhbHNlOwogICAgICAgICAgICAgaWRlbnRpZmllci0+dmFsdWUubnVt YmVyID0gaW50aWQ7CkBAIC0xNTEsNiArMTU3LDggQEAgdm9pZCBOUE5fSW5pdGlhbGl6ZVZhcmlh bnRXaXRoU3RyaW5nQ29weQogICAgIHZhcmlhbnQtPnR5cGUgPSBOUFZhcmlhbnRUeXBlX1N0cmlu ZzsKICAgICB2YXJpYW50LT52YWx1ZS5zdHJpbmdWYWx1ZS5VVEY4TGVuZ3RoID0gdmFsdWUtPlVU RjhMZW5ndGg7CiAgICAgdmFyaWFudC0+dmFsdWUuc3RyaW5nVmFsdWUuVVRGOENoYXJhY3RlcnMg PSAoTlBVVEY4ICopbWFsbG9jKHNpemVvZihOUFVURjgpICogdmFsdWUtPlVURjhMZW5ndGgpOwor ICAgIGlmICghdmFyaWFudC0+dmFsdWUuc3RyaW5nVmFsdWUuVVRGOENoYXJhY3RlcnMpCisgICAg ICAgIENSQVNIKCk7CiAgICAgbWVtY3B5KCh2b2lkKil2YXJpYW50LT52YWx1ZS5zdHJpbmdWYWx1 ZS5VVEY4Q2hhcmFjdGVycywgdmFsdWUtPlVURjhDaGFyYWN0ZXJzLCBzaXplb2YoTlBVVEY4KSAq IHZhbHVlLT5VVEY4TGVuZ3RoKTsKIH0KIApAQCAtMTgwLDcgKzE4OCw4IEBAIE5QT2JqZWN0ICpf TlBOX0NyZWF0ZU9iamVjdChOUFAgbnBwLCBOUEMKICAgICAgICAgICAgIG9iaiA9IGFDbGFzcy0+ YWxsb2NhdGUobnBwLCBhQ2xhc3MpOwogICAgICAgICBlbHNlCiAgICAgICAgICAgICBvYmogPSAo TlBPYmplY3QqKW1hbGxvYyhzaXplb2YoTlBPYmplY3QpKTsKLQorICAgICAgICBpZiAoIW9iaikK KyAgICAgICAgICAgIENSQVNIKCk7CiAgICAgICAgIG9iai0+X2NsYXNzID0gYUNsYXNzOwogICAg ICAgICBvYmotPnJlZmVyZW5jZUNvdW50ID0gMTsKIAo=