Bug 36391 – Potential for infinite recursion in SharedBuffer

• Status: NEW
• Component: Platform

[Andy Estes]

The following routine would result in infinite recursion:

SharedBuffer* sharedBufferWithCFDataAndData(CFDataRef cfData, const char* data,
unsigned dataLength)
{
    SharedBuffer* buffer = new SharedBuffer(cfData);
    buffer->append(data, dataLength);
    return buffer;
}

SharedBuffer does not appear to currently be used in this manner, but it would be valid to do so.

[Yong Li]

(In reply to comment #0)
> The following routine would result in infinite recursion:
> 
> SharedBuffer* sharedBufferWithCFDataAndData(CFDataRef cfData, const char* data,
> unsigned dataLength)
> {
>     SharedBuffer* buffer = new SharedBuffer(cfData);
>     buffer->append(data, dataLength);
>     return buffer;
> }
> 
> SharedBuffer does not appear to currently be used in this manner, but it would
> be valid to do so.

Where's the code? Also, why could it be infinite recursion?

[Andy Estes]

Take a look at SharedBufferCF.cpp and SharedBuffer.cpp.  Here is how the recursion happens:

1) The call to SharedBuffer::append() calls SharedBuffer::maybeTransferPlatformData().
2) If on a CF platform and m_cfData != 0, SharedBuffer::maybeTransferPlatformData() called SharedBuffer::append() to transfer the data in m_cfData into SharedBuffer segments.
3) SharedBuffer::append() again calls SharedBuffer::maybeTransferPlatformData().  Repeat...

[Yong Li]

(In reply to comment #2)
> Take a look at SharedBufferCF.cpp and SharedBuffer.cpp.  Here is how the
> recursion happens:
> 
> 1) The call to SharedBuffer::append() calls
> SharedBuffer::maybeTransferPlatformData().
> 2) If on a CF platform and m_cfData != 0,
> SharedBuffer::maybeTransferPlatformData() called SharedBuffer::append() to
> transfer the data in m_cfData into SharedBuffer segments.
> 3) SharedBuffer::append() again calls
> SharedBuffer::maybeTransferPlatformData().  Repeat...

Hm... so in 2), if we

if (m_cfData) {
 CFDataRef cfData(m_cfData);
 m_cfData = 0;
 append(cfData...);
}

Will this solve the problem and is it a good way?

[Andy Estes]

(In reply to comment #3)
> Hm... so in 2), if we
> 
> if (m_cfData) {
>  CFDataRef cfData(m_cfData);
>  m_cfData = 0;
>  append(cfData...);
> }
> 
> Will this solve the problem and is it a good way?

I think that would work.  I don't think you need to wrap the block with 'if (m_cfData) {}' since maybeTransferPlatformData() returns early if m_cfData is null.  Also, you'll either need to manually CFRetain() cfData before nulling out m_cfData (or wrap your new CFDataRef in a RetainPtr, like we do with m_cfData).

[Darin Adler]

(In reply to comment #3)
> if (m_cfData) {
>  CFDataRef cfData(m_cfData);
>  m_cfData = 0;
>  append(cfData...);
> }
> 
> Will this solve the problem and is it a good way?

Should be OK. I also read Andy's comments. The best idiom would be something like this:

    RetainPtr<CFDataRef> data;
    data.swap(m_cfData);
    append((const char*)CFDataGetBytePtr(data.get()), CFDataGetLength(data.get()));

Using "swap" is a technique that avoids any unnecessary CFRetain/CFRelease. It's critical to use a RetainPtr.

[Yong Li]

Andy, do you have a test that I can use to produce this problem with safari win32 build and verify the fix works? Or, do you know how I can trigger that code? If not, I will have to insert temp code somewhere to test it.

[Darin Adler]

I believe the reason that Andy used the word potential in the title of this bug is that he found it by code inspection and does not have a test case.

[Yong Li]

(In reply to comment #7)
> I believe the reason that Andy used the word potential in the title of this bug
> is that he found it by code inspection and does not have a test case.

Understand. But I have little knowledge about platform CF. We need a test case to verify this anyway.

[Andy Estes]

I agree that there should be test coverage here, but I'm not sure the best way to go about it.  Since there is no existing code path that would trigger this bug, It sounds like we need to write a unit test for SharedBuffer.  I'm not sure where unit tests would go or how they would fit in to our automated testing framework.