Bug 176803 – [SOUP] Check length before calling soup_message_body_append_buffer.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Michael Catanzaro]

Since enabling fatal criticals for layout tests, test http/tests/local/blob/send-hybrid-blob-using-open-panel.html has been crashing somewhere in the network process (there is no backtrace unfortunately) with libsoup-CRITICAL **: soup_message_body_append_buffer: assertion 'buffer->length > 0' failed. Updating expectations accordingly.

[Ms2ger (he/him; ⌚ UTC+1/+2)]

Created attachment 346763 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 346763 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346763&action=review

> Source/WebCore/ChangeLog:3
> +        [SOUP] Check length before calling soup_message_body_append_buffer.

Use the bug title here, either change it here or change the bug title.

> Source/WebCore/platform/network/soup/ResourceRequestSoup.cpp:54
> +            if (soupBuffer->length)

The thing is, is this expected to be 0, or is it 0 because of another bug? I'm afraid we might be hiding the actual bug with this patch.

[Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Carlos Garcia Campos from comment #2)
> Comment on attachment 346763 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=346763&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        [SOUP] Check length before calling soup_message_body_append_buffer.
> 
> Use the bug title here, either change it here or change the bug title.

Done.

> > Source/WebCore/platform/network/soup/ResourceRequestSoup.cpp:54
> > +            if (soupBuffer->length)
> 
> The thing is, is this expected to be 0, or is it 0 because of another bug?
> I'm afraid we might be hiding the actual bug with this patch.

I think ending up with an empty buffer here is just the logical result of reading an empty file.

[Carlos Garcia Campos]

Comment on attachment 346763 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346763&action=review

>>> Source/WebCore/platform/network/soup/ResourceRequestSoup.cpp:54
>>> +            if (soupBuffer->length)
>> 
>> The thing is, is this expected to be 0, or is it 0 because of another bug? I'm afraid we might be hiding the actual bug with this patch.
> 
> I think ending up with an empty buffer here is just the logical result of reading an empty file.

It make sense. If it's expected, then I guess there's no point in creating an empty SoupBuffer just to check that is empty. I think we could do something like:

if (blobItem.length() == BlobDataItem::toEndOfFile)
    return 0;

GUniquePtr<SoupBuffer> soupBuffer(buffer->createSoupBuffer(blobItem.offset(), blobItem.length()));
soup_message_body_append_buffer(soupMessage->request_body, soupBuffer.get());
return soupBuffer->length;

[Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Carlos Garcia Campos from comment #4)
> Comment on attachment 346763 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=346763&action=review
> 
> >>> Source/WebCore/platform/network/soup/ResourceRequestSoup.cpp:54
> >>> +            if (soupBuffer->length)
> >> 
> >> The thing is, is this expected to be 0, or is it 0 because of another bug? I'm afraid we might be hiding the actual bug with this patch.
> > 
> > I think ending up with an empty buffer here is just the logical result of reading an empty file.
> 
> It make sense. If it's expected, then I guess there's no point in creating
> an empty SoupBuffer just to check that is empty. I think we could do
> something like:
> 
> if (blobItem.length() == BlobDataItem::toEndOfFile)
>     return 0;
> 
> GUniquePtr<SoupBuffer>
> soupBuffer(buffer->createSoupBuffer(blobItem.offset(), blobItem.length()));
> soup_message_body_append_buffer(soupMessage->request_body, soupBuffer.get());
> return soupBuffer->length;

That's not what a zero size argument to createSoupBuffer means; it uses

> size ? size : this->size()

for some odd reason.

[Carlos Garcia Campos]

(In reply to Ms2ger from comment #5)
> (In reply to Carlos Garcia Campos from comment #4)
> > Comment on attachment 346763 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=346763&action=review
> > 
> > >>> Source/WebCore/platform/network/soup/ResourceRequestSoup.cpp:54
> > >>> +            if (soupBuffer->length)
> > >> 
> > >> The thing is, is this expected to be 0, or is it 0 because of another bug? I'm afraid we might be hiding the actual bug with this patch.
> > > 
> > > I think ending up with an empty buffer here is just the logical result of reading an empty file.
> > 
> > It make sense. If it's expected, then I guess there's no point in creating
> > an empty SoupBuffer just to check that is empty. I think we could do
> > something like:
> > 
> > if (blobItem.length() == BlobDataItem::toEndOfFile)
> >     return 0;
> > 
> > GUniquePtr<SoupBuffer>
> > soupBuffer(buffer->createSoupBuffer(blobItem.offset(), blobItem.length()));
> > soup_message_body_append_buffer(soupMessage->request_body, soupBuffer.get());
> > return soupBuffer->length;
> 
> That's not what a zero size argument to createSoupBuffer means; it uses
> 
> > size ? size : this->size()
> 
> for some odd reason.

This is because you can create a sub-buffer, that's why it receives an offset and a size. If the size is 0, then the actual buffer size is used. This was added before std::optional. In this case, if the file is empty the buffer size will be 0 in any case. We can change the condition to:

if (buffer->isEmpty())
    return 0;

That way we are actually checking that it's an empty file. Previous check was indeed wrong.

[Ms2ger (he/him; ⌚ UTC+1/+2)]

Created attachment 347488 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 347488 [details]
Patch

Clearing flags on attachment: 347488

Committed r235026: <https://trac.webkit.org/changeset/235026>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.