After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 670518 - byte range request is not handled properly
byte range request is not handled properly
Status: RESOLVED FIXED
Product: GUPnP
Classification: Other
Component: gupnp
0.18.x
Other Linux
: Normal normal
: ---
Assigned To: GUPnP Maintainers
GUPnP Maintainers
Depends on:
Blocks:
 
 
Reported: 2012-02-21 08:48 UTC by Lukasz Pawlik
Modified: 2012-02-24 14:05 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch proposal (3.41 KB, patch)
2012-02-21 08:48 UTC, Lukasz Pawlik
none Details | Review
Add test for byte-range requests (8.58 KB, patch)
2012-02-22 11:01 UTC, Jens Georg
none Details | Review
Include test file in previous patch (14.07 KB, patch)
2012-02-22 11:05 UTC, Jens Georg
none Details | Review
Add test for byte-range requests (13.98 KB, patch)
2012-02-22 11:18 UTC, Jens Georg
committed Details | Review
patch proposal with functions from libsoup (5.33 KB, patch)
2012-02-23 08:37 UTC, Lukasz Pawlik
committed Details | Review

Description Lukasz Pawlik 2012-02-21 08:48:17 UTC
Created attachment 208096 [details] [review]
patch proposal

Steps to reproduce: 
1. Run rygel 
2. Send GET request with Range in header (i.e. bytes=0-0) for a device description (netcat will be useful)
3. Observe results

Actual outcome:
For bytes=0-0 servers results whole content of requested file.
For byte=33-66 server returns 33 bytes in body but sets Content-Length: 99 in response header.

Expected outcome:
for bytes=0-0 return first byte from the file content 
for bytes=33-66 set Content-Length:33 in response header
Comment 1 Lukasz Pawlik 2012-02-21 09:30:09 UTC
Alternative solution for this problem is porting libsoup function. They seems to be over-engineered for our needs as they allow to handle multipart/byteranges and we do not want to support this in gupnp so for the sake of simplicity we should stay with current implementation.
Comment 2 Jens Georg 2012-02-22 11:01:41 UTC
Created attachment 208186 [details] [review]
Add test for byte-range requests

The range requests seem a bit more broken than that. See the attached unittest.
Comment 3 Jens Georg 2012-02-22 11:05:04 UTC
Created attachment 208187 [details] [review]
Include test file in previous patch
Comment 4 Jens Georg 2012-02-22 11:18:08 UTC
Created attachment 208188 [details] [review]
Add test for byte-range requests

Remove check for the length returned by soup_message_headers_get_content_range as that's the total length.
Comment 5 Lukasz Pawlik 2012-02-23 08:37:02 UTC
Created attachment 208234 [details] [review]
patch proposal with functions from libsoup

Attached test pass with this version of patch.
Comment 6 Jens Georg 2012-02-24 10:01:24 UTC
Review of attachment 208234 [details] [review]:

Looks fine apart from the one comment.

::: libgupnp/gupnp-context.c
@@ +933,3 @@
+                /* We do not support mulipart/byteranges so only first first */
+                /* range from request is handled */
+                                                     &nranges))

Wonder if we shouldn't return NOT_SATISFIABLE on multipart/byterange. Need to look that up in the RFC.
Comment 7 Jens Georg 2012-02-24 10:01:26 UTC
Review of attachment 208234 [details] [review]:

Looks fine apart from the one comment.

::: libgupnp/gupnp-context.c
@@ +933,3 @@
+                /* We do not support mulipart/byteranges so only first first */
+                /* range from request is handled */
+                                                     &nranges))

Wonder if we shouldn't return NOT_SATISFIABLE on multipart/byterange. Need to look that up in the RFC.
Comment 8 Jens Georg 2012-02-24 13:43:32 UTC
Review of attachment 208234 [details] [review]:

::: libgupnp/gupnp-context.c
@@ +926,3 @@
 
+                if (soup_message_headers_get_ranges (msg->request_headers,
+                                                     msg->response_body->length,

this needs to be st.st_size. msg->response_body->length is 0 here, leading to ranges that may have negative start and or end
Comment 9 Jens Georg 2012-02-24 14:05:22 UTC
Fix pushed with minor change (using st.st_size in _get_ranges). Don't know about the multipar stuff, but at least we're consistent with how it was before.