Summary: | [SOUP] Check length before calling soup_message_body_append_buffer. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||||
Component: | WebKitGTK | Assignee: | Ms2ger (he/him; ⌚ UTC+1/+2) <Ms2ger> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | berto, bugs-noreply, cgarcia, commit-queue, ews-watchlist, gustavo, Ms2ger | ||||||
Priority: | P2 | ||||||||
Version: | Other | ||||||||
Hardware: | PC | ||||||||
OS: | Linux | ||||||||
Attachments: |
|
Description
Michael Catanzaro
2017-09-12 13:22:42 PDT
Created attachment 346763 [details]
Patch
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. (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. 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; (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. (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. Created attachment 347488 [details]
Patch
Comment on attachment 347488 [details] Patch Clearing flags on attachment: 347488 Committed r235026: <https://trac.webkit.org/changeset/235026> All reviewed patches have been landed. Closing bug. |