Bug 19287 - return value of malloc() is not checked in npruntime.cpp
Summary: return value of malloc() is not checked in npruntime.cpp
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-05-28 01:58 PDT by Peter Siket
Modified: 2009-01-11 02:27 PST (History)
3 users (show)

See Also:


Attachments
proposed patch (2.98 KB, patch)
2008-12-01 06:57 PST, Gabriella Toth
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Siket 2008-05-28 01:58:53 PDT
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);
Comment 1 Alexey Proskuryakov 2008-05-28 05:31:03 PDT
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.
Comment 2 Eric Seidel (no email) 2008-05-28 08:45:22 PDT
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. :)
Comment 3 Darin Adler 2008-05-28 08:59:04 PDT
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.
Comment 4 Gabriella Toth 2008-12-01 06:56:22 PST
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.
Comment 5 Gabriella Toth 2008-12-01 06:57:20 PST
Created attachment 25630 [details]
proposed patch
Comment 6 Anders Carlsson 2008-12-01 10:06:48 PST
Comment on attachment 25630 [details]
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.
Comment 7 Gabriella Toth 2008-12-01 23:56:21 PST
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?
Comment 8 Darin Adler 2008-12-15 06:20:39 PST
(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.
Comment 9 Darin Adler 2008-12-15 06:43:25 PST
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.
Comment 10 Darin Adler 2008-12-15 07:07:00 PST
(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.
Comment 11 Anders Carlsson 2008-12-15 10:52:53 PST
(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?
Comment 12 Darin Adler 2008-12-15 11:18:29 PST
(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."
Comment 13 Darin Adler 2009-01-02 10:53:36 PST
Comment on attachment 25630 [details]
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.
Comment 14 Alexey Proskuryakov 2009-01-11 02:27:17 PST
Committed revision 39792.