[webkit-reviews] review denied: [Bug 27777] ImageSourceCG makes bad data refs (race condition causes blank images) : [Attachment 43462] Patch to fix crash

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Avi Drissman <avi at drissman.com>'s
request for review:
Bug 27777: ImageSourceCG makes bad data refs (race condition causes blank
images)
https://bugs.webkit.org/show_bug.cgi?id=27777

Attachment 43462: Patch to fix crash
https://bugs.webkit.org/attachment.cgi?id=43462&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Looks like a great fix!

Is there any way to test this? How did you discover this mistake?

> +	   (WebCore::):

Please remove lines like this from the ChangeLog when prepare-ChangeLog puts
them in.

> +    ssize_t amount = std::min(static_cast<off_t>(count),
sharedBuffer->size() - position);

We normally use "using namespace std" at the top of the file and call functions
as just "min" rather than "std::min".

I don't think ssize_t exists on Windows. I suggest just using off_t for the
local variable.

You can use min<off_t> instead of casting count to off_t if you like.

> +    if (amount <= 0)
> +	   return 0;

This seems unnecessary. The memcpy function already does the right thing when
passed a zero. There is no chance of a negative number here, is there?

If we really do need to handle the case where position is greater than the
buffer size, I think we should check that specifically by comparing size and
position before substracting. Then we can use the type size_t within the
function and avoid the signed arithmetic in the min expression.

> +// We use the GetBytesAtPosition callback rather than the GetBytePointer one
because SharedBuffer
> +// does not provide a way to lock down the byte pointer and guarantee that
it won't move, which
> +// is a requirement for using the GetBytePointer callback.
> +static const CGDataProviderDirectCallbacks kProviderCallbacks = {0, NULL,
NULL, sharedBufferGetBytesAtPosition, sharedBufferRelease};

Can we just put this inside the function, instead of at namespace scope?
There's no need for it to be a global. The old idiom used for the allocator
seems cleaner to me.

We don't use "k" for constants in WebKit.

I know that the callbacks in the Core Foundation library itself use this style
with the "k", but we don't.

We use "0" rather than NULL in WebKit.

We normally put spaces after the first "{" and before the last "}".

Would you be willing to do the same fix in PDFDocumentImage?

review- mainly because of the minor coding style issues