Bug 96272 – NPN_InitializeVariantWithStringCopy is wrong for platforms that return NULL from malloc(0)

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

[Julien Brianceau]

NPN_InitializeVariantWithStringCopy function call CRASH macro when NPString passed in parameters has a null length and malloc(0) returns NULL.
CRASH should only be called in this function for malloc failures with NPString length greater than 0.

[Julien Brianceau]

Created attachment 163139 [details]
NPN_InitializeVariantWithStringCopy patch

[WebKit Review Bot]

Attachment 163139 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/bridge/npruntime.cpp:95:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Julien Brianceau]

Created attachment 163142 [details]
NPN_InitializeVariantWithStringCopy patch (2)

[Alexey Proskuryakov]

Comment on attachment 163142 [details]
NPN_InitializeVariantWithStringCopy patch (2)

I wonder if we can just switch to fastMalloc/fastFree here. It doesn't look like a plug-in could directly invoke free() on the pointer.

Unlike malloc/free, fastMalloc has well defined behavior when size is 0.

[Julien Brianceau]

(In reply to comment #4)
I don't know if we can switch to fastMalloc/fastFree here.
Perhaps it would be good to propose this switch in a separate new changeset ?

[Darin Adler]

(In reply to comment #5)
> I don't know if we can switch to fastMalloc/fastFree here.

I think we can.

> Perhaps it would be good to propose this switch in a separate new changeset ?

Perhaps, but maybe you misunderstand what Alexey meant.

Alexey’s point is that changing to fastMalloc/fastFree would be an alternate way to fix this very bug. Not that it would be an interesting bug unrelated change.

That fix would not require adding a special case for empty strings. Since we don’t have a good reason to optimize empty strings, it would be nice to fix this in a way that would not require making this otherwise-simple function more complex.

[Julien Brianceau]

Comment on attachment 163142 [details]
NPN_InitializeVariantWithStringCopy patch (2)

Ok, I'll provide a new patch to use fastMalloc allocator for NPN objects and string variants, and perform few tests.
But I see that even fastMalloc can return 0 when USE_SYSTEM_MALLOC define is set in Source/WTF/wtf/Platform.h. Should we handle this case ?

[Darin Adler]

(In reply to comment #7)
> But I see that even fastMalloc can return 0 when USE_SYSTEM_MALLOC define is set in Source/WTF/wtf/Platform.h.

That is incorrect.

Here is the code from FastMalloc.cpp (I removed the WTF_MALLOC_VALIDATION #if for clarity):

    void* fastMalloc(size_t n) 
    {
        ASSERT(!isForbidden());
        void* result = malloc(n);
        if (!result)
            CRASH();
        return result;
    }

The function will crash if malloc returns zero. So this implementation is not compatible with systems where the system malloc can return 0 when the passed-in size is 0. If someone needs to use USE_SYSTEM_MALLOC on a system like that, they’ll need to fix that in FastMalloc.cpp; it’s not something we have to worry about elsewhere.

[Julien Brianceau]

(In reply to comment #8)
> (In reply to comment #7)
> > But I see that even fastMalloc can return 0 when USE_SYSTEM_MALLOC define is set in Source/WTF/wtf/Platform.h.
> 
> That is incorrect.
> 

Aw yes you're right. I looked at tryFastMalloc implementation thinking it was fastMalloc, my mistake.

[Julien Brianceau]

Mmmh this seems to be more complex that I thought.

If I understand correctly NetScape plugins, a plugin can allocate its own String variant through NPN_InitializeVariantWithString call. In this case, the caller allocate the String, and this String will be released through NPReleaseVariantValue.

I'm clearly not an expert in NetScape plugins but I'm pretty sure that I'll introduce side effects crashes if I switch this to fastMalloc/fastFree.

[Darin Adler]

(In reply to comment #4)
> I wonder if we can just switch to fastMalloc/fastFree here.

Given Julien’s most recent comment, it seems we can’t.

[Alexey Proskuryakov]

> If I understand correctly NetScape plugins, a plugin can allocate its own String variant through NPN_InitializeVariantWithString call.

Do we implement that in WebKit? I could not find it anywhere in source code, only NPN_InitializeVariantWithStringCopy, which of course copies the string with any WebKit provided allocator.

[Darin Adler]

(In reply to comment #12)
> > If I understand correctly NetScape plugins, a plugin can allocate its own String variant through NPN_InitializeVariantWithString call.
> 
> Do we implement that in WebKit? I could not find it anywhere in source code, only NPN_InitializeVariantWithStringCopy, which of course copies the string with any WebKit provided allocator.

Lets get to the bottom of this. Either fix (fastMalloc, or adding zero check) seems OK, but we should do the fastMalloc fix if we can.

[Julien Brianceau]

(In reply to comment #13)
> 
> Lets get to the bottom of this. Either fix (fastMalloc, or adding zero check) seems OK, but we should do the fastMalloc fix if we can.

I agree. I removed obsolete flag on my patch and let someone more skilled than me in NPAPI decide what to do here. Of course I won't be offended at all if this patch is dropped to switch to fastMalloc :-)

[Alexey Proskuryakov]

Julien, can you please elaborate on your comment 10? I think that this is what preventing us from using fastMalloc to solve this siiue.

[Julien Brianceau]

(In reply to comment #15)
> Julien, can you please elaborate on your comment 10? I think that this is what preventing us from using fastMalloc to solve this siiue.

I didn't find an implementation of NPN_InitializeVariantWithString in current versions of WebKit, so my comment #10 is useless.

However there is still "STRINGZ_TO_NPVARIANT" macro (in Source/WebCore/plugins/npruntime.h), and I read this: http://stackoverflow.com/questions/1431409/firefox-npapi-plugin-development-firefox-freeze-when-calling-a-method
So I thought that we could perhaps switch to NPN_MemAlloc/NPN_MemFree calls, then switch these 2 functions' implementation to fastMalloc/fastFree (in Source/WebCore/plugins/npapi.cpp).

But it seems that there's quite an history for this: https://bugs.webkit.org/show_bug.cgi?id=20566, and I still can see this kind of comment (FIXME: This should really call NPN_MemAlloc but that's in WebKit) in Source/WebCore/bridge/NP_jsobject.cpp

[Alexey Proskuryakov]

Oh, interesting. So plug-ins can get their own data into a variant with STRINGZ_TO_NPVARIANT. The contract is that they use NPN_MemAlloc, but we don't really want to rely on that, as a comment in npnMemAlloc explains:

void* npnMemAlloc(uint32_t size)
{
    // We could use fastMalloc here, but there might be plug-ins that mix NPN_MemAlloc/NPN_MemFree with malloc and free,
    // so having them be equivalent seems like a good idea.
    return malloc(size);
}

This means that it's undesirable to switch NPN_InitializeVariantWithStringCopy to fastMalloc indeed. But we also need a comment explaining this, so that the next time someone looks at this code, they could find the explanation more easily.

[Julien Brianceau]

Created attachment 184453 [details]
NPN_InitializeVariantWithStringCopy patch (3)

Comment added in the code + ChangeLog update

[Darin Adler]

Comment on attachment 184453 [details]
NPN_InitializeVariantWithStringCopy patch (3)

View in context: https://bugs.webkit.org/attachment.cgi?id=184453&action=review

> Source/WebCore/ChangeLog:10
> +        No new tests. This is platform dependant.

Spelling error: dependent is not spelled with an "a"

> Source/WebCore/bridge/npruntime.cpp:90
> +        // switching to fastMalloc would be better to avoid length check but this is not desirable
> +        // as NPN_MemAlloc is using malloc and there might be plugins that mix NPN_MemAlloc and malloc too

WebKit comment coding style is “sentence style”. First letter capitalized, period at end of sentence.

[Julien Brianceau]

Created attachment 184523 [details]
NPN_InitializeVariantWithStringCopy patch (4)

[Alexey Proskuryakov]

Comment on attachment 184523 [details]
NPN_InitializeVariantWithStringCopy patch (4)

View in context: https://bugs.webkit.org/attachment.cgi?id=184523&action=review

This is going for way too long already, but I have additional comments, sorry about that.

> Source/WebCore/ChangeLog:8
> +        See bug 96272 comments for further information.

This is not helpful - the bug number is the same as above, so of course one would go there for additional information.

> Source/WebCore/bridge/npruntime.cpp:91
> +        variant->value.stringValue.UTF8Characters = (NPUTF8 *)malloc(sizeof(NPUTF8) * value->UTF8Length);

This is a pre-existing error, but WebKit style is to put star next to C++ class name (so it would be "(NPUTF8*)). It's also WebKit style to use static_cast over C-style casts, although I personally don't see that as beneficial in cases like this.

> Source/WebCore/bridge/npruntime.cpp:93
> +        if (!variant->value.stringValue.UTF8Characters)
> +            CRASH();

Can't we just change this check to the one below?

if (value->UTF8Length && !variant->value.stringValue.UTF8Characters)
    CRASH();

That would be a much smaller change, and more readable code, I think.

[Julien Brianceau]

Created attachment 184586 [details]
 NPN_InitializeVariantWithStringCopy patch (5)

Changes performed as requested excepting for the static_cast instead of C-style cast (because there are others C-style cast in this file for malloc/free calls and I'd prefer that we don't mix the bug fix with style changes).

[WebKit Review Bot]

Comment on attachment 184586 [details]
 NPN_InitializeVariantWithStringCopy patch (5)

Clearing flags on attachment: 184586

Committed r140751: <http://trac.webkit.org/changeset/140751>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.