Bug 19690 – SharedBuffer::buffer() fails with platform data

• Component: Platform
• Status: UNCONFIRMED

[Avi Drissman]

SharedBuffer has the ability (on the Mac) to have a "platform data" member, so that if we want to have a SharedBuffer created from, say, an NSData object, we can do so, and all the member functions treat it as if the data were actually in m_buffer.

This is achieved in data() and size() by checking with hasPlatformData(), and in append() by calling maybeTransferPlatformData(). However, SharedBuffer::buffer() does not properly wrap the platform data's existence. This failure is an accident waiting to happen.

To repro:
- Create a SharedBuffer with an NSData
- Call data()
-- result: the correct data pointer
- Call size()
-- result: the correct data size
- Call buffer()
-- result: an empty buffer

A reasonable patch would be:

-    const Vector<char> &buffer() { return m_buffer; }
+    const Vector<char> &buffer() { maybeTransferPlatformData(); return m_buffer; }

Perhaps it could be optimized if buffer() must be on the fast path, but correctness is always preferable to speed.

[Avi Drissman]

Created attachment 21857 [details]
Patch to fix the buffer() call to properly use the platform data if available

[Darin Adler]

Anders, do you think this is the right fix?

As long as we're patching that line, we really need to move the "&" one to the left.

[Anders Carlsson]

Comment on attachment 21857 [details]
Patch to fix the buffer() call to properly use the platform data if available

The patch looks good, but I'm curious where we call buffer() on a wrapped SharedBuffer. 

If this is not yet a problem, I'd be more happy with an ASSERT(!hasPlatformData()).

[Avi Drissman]

Do we call buffer() at all?

- If not, then we shouldn't have an interface that's not used and buffer() should be dropped.
- If so, then just because today we don't happen to call it while there's platform data doesn't mean tomorrow we won't. I'm not too fond of putting in an assert to catch a reasonable usage that happens to give the wrong result.

[Eric Seidel (no email)]

Comment on attachment 21857 [details]
Patch to fix the buffer() call to properly use the platform data if available

Assigning this to Anders for review.

[Anders Carlsson]

Comment on attachment 21857 [details]
Patch to fix the buffer() call to properly use the platform data if available

 I think that we should state that calling buffer() on a SharedBuffer that wraps platform data is invalid, and add an assert.

I don't blindly want to add a maybeTransferPlatformData() because that is a potentially expensive operation and we'd like to avoid that, especially since buffer() returns a const reference.