Bug 202152 – Address static analysis warning in Allocator.cpp: Null pointer argument in call to memory copy function

• Status: RESOLVED
• Resolution: FIXED
• Component: bmalloc

[Keith Rollin]

Xcode's static analysis facility flags the following:

    .../OpenSource/Source/bmalloc/bmalloc/Allocator.cpp:98:5: warning: Null pointer argument in call to memory copy function
        memcpy(result, object, copySize);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

https://en.cppreference.com/w/cpp/string/byte/memcpy explains that this is undefined behavior:


    "If either dest or src is a null pointer, the behavior is undefined, even if count is zero."

I suppose that passing in a null source pointer could be bad if the implementation fetched the first source byte to copy before actually checking the number of bytes to copy. So far, it doesn’t seem to be an issue, but we should clean this up.

Simply adding "if (result && object)" before the memcpy will add tests and branches in the hot path of this function and so might not be the best solution. Instead, add a little bit of duplicate code earlier in the function at a place where we already know that "object" is NULL.

[Radar WebKit Bug Importer]

<rdar://problem/55671444>

[Keith Rollin]

Created attachment 379463 [details]
Patch

[Geoffrey Garen]

Comment on attachment 379463 [details]
Patch

I kinda think "if (!object) return allocateImpl(newSize, action);" at the top of the function would read better. Then you can remove the ASSERT that nullptr is a small object too. This function is starting too feel a little too branch-y and early return-y to follow. And there shouldn't be any cost to an up-front null check.

[Keith Rollin]

Created attachment 379492 [details]
Patch

[Keith Rollin]

Updated as Geoff describes.

[Geoffrey Garen]

Comment on attachment 379492 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 379492 [details]
Patch

Clearing flags on attachment: 379492

Committed r250325: <https://trac.webkit.org/changeset/250325>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.