Bug 149011 – Fix null dereferences from static analysis

• Status: NEW
• Component: WebKit Misc.

[Alex Christensen]

I guess this will make things better.

[Alex Christensen]

Created attachment 260876 [details]
Patch

[Alex Christensen]

Created attachment 260881 [details]
Patch

[Darin Adler]

Comment on attachment 260881 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:3
> +        Fix null dereferences from static analysis

You mean “fix null dereferences detected by static analysis tool”?

> Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp:188
> -    RELEASE_ASSERT(record->patchpointID == exit.m_stackmapID);
> +    RELEASE_ASSERT(record && record->patchpointID == exit.m_stackmapID);

In a normal assert we would have done two separate ones, so we could tell of the two && was failing. Not sure if that thinking applies to RELEASE_ASSERT.

> Source/WTF/wtf/ParkingLot.cpp:330
> +    RELEASE_ASSERT(oldHashtable && newSize > oldHashtable->size);

Same comment about && in assertions.

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:623
> -    if (isRemoved())
> +    if (!m_source)
>          return;

Isn’t the static analyzer smart enough to understand that this is the same thing?

> Source/WebCore/contentextensions/CombinedURLFilters.cpp:273
> -            ASSERT_WITH_MESSAGE(sourceActiveSubtree, "The root should always have a valid generator.");
> +            RELEASE_ASSERT_WITH_MESSAGE(sourceActiveSubtree, "The root should always have a valid generator.");

This doesn’t seem like a good change to me. How is adding a null check at runtime helpful to the project?

> Source/WebCore/css/SelectorCheckerTestFunctions.h:-185
> -        unsigned rangeSubtagsEndIndex = rangeLength;

This is not a fix to a null dereference.

> Source/WebCore/rendering/CounterNode.cpp:84
> +                    if (nextSibling)
> +                        nextSibling->m_previousSibling = child;

I do think this is a real bug. Can we figure out how to make a test case for this?

[Alexey Proskuryakov]

I think that it's important to tell which part of release assert fails. Otherwise, investigating frequent crashes would be unnecessarily harder.

[Ahmad Saleem]

Still can be applied:

>> CounterNode.cpp:

https://searchfox.org/wubkat/rev/711120e7edec012527620d07bf63d85713a180fd/Source/WebCore/rendering/CounterNode.cpp#78

>> ParkingLot.cpp:

https://searchfox.org/wubkat/rev/711120e7edec012527620d07bf63d85713a180fd/Source/WTF/wtf/ParkingLot.cpp#384

>> SourceBuffer.cpp:

https://searchfox.org/wubkat/rev/711120e7edec012527620d07bf63d85713a180fd/Source/WebCore/Modules/mediasource/SourceBuffer.cpp#655

>> CombinedURLFilters.cpp:

https://searchfox.org/wubkat/rev/711120e7edec012527620d07bf63d85713a180fd/Source/WebCore/contentextensions/CombinedURLFilters.cpp#227

>> 

___

Can't find - FTLOSRExitCompiler.cpp change and TestNetscapePlugIn/main.cpp.

This is not needed in SourceBuffer.cpp:

https://searchfox.org/wubkat/rev/711120e7edec012527620d07bf63d85713a180fd/Source/WebCore/Modules/mediasource/SourceBuffer.cpp#687

This is not needed in SelectorCheckerTestFunctions.h:

https://searchfox.org/wubkat/rev/711120e7edec012527620d07bf63d85713a180fd/Source/WebCore/css/SelectorCheckerTestFunctions.h#243

___

Just wanted to update.