Bug 19690 - SharedBuffer::buffer() fails with platform data
Summary: SharedBuffer::buffer() fails with platform data
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 525.x (Safari 3.1)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-06-20 07:58 PDT by Avi Drissman
Modified: 2010-06-10 15:51 PDT (History)
2 users (show)

See Also:


Attachments
Patch to fix the buffer() call to properly use the platform data if available (483 bytes, patch)
2008-06-20 10:58 PDT, Avi Drissman
andersca: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Avi Drissman 2008-06-20 07:58:07 PDT
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.
Comment 1 Avi Drissman 2008-06-20 10:58:46 PDT
Created attachment 21857 [details]
Patch to fix the buffer() call to properly use the platform data if available
Comment 2 Darin Adler 2008-06-23 10:49:50 PDT
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.
Comment 3 Anders Carlsson 2008-06-23 11:04:41 PDT
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()).
Comment 4 Avi Drissman 2008-07-21 12:37:14 PDT
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.
Comment 5 Eric Seidel (no email) 2008-08-01 15:03:58 PDT
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.
Comment 6 Anders Carlsson 2008-08-01 15:09:22 PDT
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.