Bug 19287 – return value of malloc() is not checked in npruntime.cpp

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[Peter Siket]

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);

[Alexey Proskuryakov]

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.

[Eric Seidel (no email)]

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. :)

[Darin Adler]

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.

[Gabriella Toth]

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.

[Gabriella Toth]

Created attachment 25630 [details]
proposed patch

[Anders Carlsson]

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.

[Gabriella Toth]

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?

[Darin Adler]

(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.

[Darin Adler]

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.

[Darin Adler]

(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.

[Anders Carlsson]

(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?

[Darin Adler]

(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."

[Darin Adler]

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.

[Alexey Proskuryakov]

Committed revision 39792.