Thread Rating:
  • 0 Votes - 0 Average
  • 1
  • 2
  • 3
  • 4
  • 5
Problem when action request doesn't have Content-Length header
04-07-2013, 08:32 AM
Post: #11
RE: Problem when action request doesn't have Content-Length header
(04-07-2013 08:12 AM)simonc Wrote:  Thanks for doing this. I agree that it isn't valid for a HTTP/1.1 client to send a request without either a Content-Length: or Transfer-Encoding: Chunked header. I think it is still valid for a HTTP/1.0 client to connect to ohNet's 1.1 server and do this however (rfc2616 covers HTTP/1.1 only).

Unfortunately, this isn't correct. I have checked the HTTP 1.0 specification (this document), and section 7.2 of says:

An entity body is included with a request message only when the request method calls for one. The presence of an entity body in a request is signaled by the inclusion of a Content-Length header field in the request message headers. HTTP/1.0 requests containing an entity body must include a valid Content-Length header field.

This means that the device code that sends event NOTIFY messages to HTTP 1.0 control points needs to be changed to include a Content-Length header. If you are OK with this change, I will work on producing a patch.

Also, section 7.2.2 of the same document says:

Closing the connection cannot be used to indicate the end of a request body, since it leaves no possibility for the server to send back a response. Therefore, HTTP/1.0 requests containing an entity body must include a valid Content-Length header field. If a request contains an entity body and Content-Length is not specified, and the server does not recognize or cannot calculate the length from other fields, then the server should send a 400 (bad request) response.

This means that an ohNet device should not accept a POST request without a Content-Length header from an HTTP 1.0 control point, but should send a 400 Bad Request response.

Quote:Oh, and the original bug noted in this thread should be fixed and on github now. Smile

Thanks very much! I'll look at the code later.
Find all posts by this user
04-07-2013, 09:00 AM
Post: #12
RE: Problem when action request doesn't have Content-Length header
(04-07-2013 08:32 AM)simoncn Wrote:  
(04-07-2013 08:12 AM)simonc Wrote:  Thanks for doing this. I agree that it isn't valid for a HTTP/1.1 client to send a request without either a Content-Length: or Transfer-Encoding: Chunked header. I think it is still valid for a HTTP/1.0 client to connect to ohNet's 1.1 server and do this however (rfc2616 covers HTTP/1.1 only).

Unfortunately, this isn't correct. I have checked the HTTP 1.0 specification (this document), and section 7.2 of says:
....
This means that the device code that sends event NOTIFY messages to HTTP 1.0 control points needs to be changed to include a Content-Length header. If you are OK with this change, I will work on producing a patch.

Thanks for the link, you're correct that current publisher behaviour with HTTP/1.0 subscribers is invalid. I think the implementation doesn't even do what we expect either - we don't actually close the connection after sending the request but instead block waiting for a response from the subscriber. Won't this hang for any subscriber which really waits for connection close rather than reading until it finds the end of the event xml?

Thanks for the offer of preparing a patch - that'd be great. Being awkward, I'd like to avoid any new dynamic memory allocation on the device side. This is likely to make the implementation a bit fiddly. Feel free to tell me to fix it myself if you prefer Smile.
Find all posts by this user
04-07-2013, 09:32 AM
Post: #13
RE: Problem when action request doesn't have Content-Length header
(04-07-2013 09:00 AM)simonc Wrote:  Thanks for the link, you're correct that current publisher behaviour with HTTP/1.0 subscribers is invalid. I think the implementation doesn't even do what we expect either - we don't actually close the connection after sending the request but instead block waiting for a response from the subscriber. Won't this hang for any subscriber which really waits for connection close rather than reading until it finds the end of the event xml?

Yes, it would, and this is presumably why the HTTP 1.0 rules are written as they are. Smile

Quote:Thanks for the offer of preparing a patch - that'd be great. Being awkward, I'd like to avoid any new dynamic memory allocation on the device side. This is likely to make the implementation a bit fiddly. Feel free to tell me to fix it myself if you prefer Smile.

I think it would be necessary to write the complete event XML data into a buffer (or buffers), then write a Content-Length header containing the length of the buffer data, then write the buffer data. If the XML data length exceeds kMaxRequestBytes (12 * 1024), it would not fit into iWriteBuffer, so an additional write buffer (perhaps more than one) would need to be created.

Does this additional write buffer creation violate the requirement for no dynamic allocation? If so, how can this be avoided?
Find all posts by this user
04-07-2013, 10:07 AM (This post was last modified: 04-07-2013 10:08 AM by simonc.)
Post: #14
RE: Problem when action request doesn't have Content-Length header
(04-07-2013 09:32 AM)simoncn Wrote:  I think it would be necessary to write the complete event XML data into a buffer (or buffers), then write a Content-Length header containing the length of the buffer data, then write the buffer data. If the XML data length exceeds kMaxRequestBytes (12 * 1024), it would not fit into iWriteBuffer, so an additional write buffer (perhaps more than one) would need to be created.

Does this additional write buffer creation violate the requirement for no dynamic allocation? If so, how can this be avoided?

Yes, this would constitute additional/undesirable allocation. Off the top of my head, options to avoid it include
  • Two passes through the event xml - the first discarding content but keeping track of the lengths written, allowing us to calculate the content length before sending the request body. There might be some neat implementations of this involving writing custom IWriter-derived classes.
  • A single (or pool of) pre-allocated event xml buffers that all PropertyWriterUpnp instances use to write xml for HTTP/1.0 subscribers.
  • Assume we can get away with violating the 1.0 spec and close our connection after writing our request, ignoring any response (PropertyWriterUpnp::PropertyWriteEnd effectively ignores the response anyway)
  • Treat the size of iWriteBuffer as the maximum size of evented update we support, increasing the buffer size if necessary.
Writing this reminds me about a bug concerning dynamic allocation of PropertyWriterUpnp instances. It might be nice to tackle this at the same time.

Since this task is turning into a requirement to fix up several layers of my buggy code, would it be fairer if I took it on?
Find all posts by this user
04-07-2013, 11:07 AM (This post was last modified: 04-07-2013 11:11 AM by simoncn.)
Post: #15
RE: Problem when action request doesn't have Content-Length header
(04-07-2013 10:07 AM)simonc Wrote:  Yes, this would constitute additional/undesirable allocation. Off the top of my head, options to avoid it include

Two passes through the event xml - the first discarding content but keeping track of the lengths written, allowing us to calculate the content length before sending the request body. There might be some neat implementations of this involving writing custom IWriter-derived classes.

I thought of this, but I wasn't sure if calling the methods that write the data would have any side-effects such as marking the updates as having been sent. Also, is it possible that the event data could change between the first write pass and the second write pass?

Quote:A single (or pool of) pre-allocated event xml buffers that all PropertyWriterUpnp instances use to write xml for HTTP/1.0 subscribers.

This seems OK. Instead of preallocating this pool, the pool size could be adjusted dynamically to grow to the largest size of XML data that needs its length to be computed. This is still dynamic allocation, but it isn't done on a per-event basis.

Quote:Assume we can get away with violating the 1.0 spec and close our connection after writing our request, ignoring any response (PropertyWriterUpnp::PropertyWriteEnd effectively ignores the response anyway)

Violating the spec and risking interoperability problems seems undesirable. I'd prefer one of the other options if possible.

Quote:Treat the size of iWriteBuffer as the maximum size of evented update we support, increasing the buffer size if necessary.

This might be OK, especially if another notification event could be generated immediately afterwards for any property changes that couldn't be accommodated in the first event notification message.

Edit: A possible problem with this is that a property change that exceeds the write buffer length (e.g., a large string value) couldn't be sent at all, because it doesn't fit in a single write buffer.

Quote:Writing this reminds me about a bug concerning dynamic allocation of PropertyWriterUpnp instances. It might be nice to tackle this at the same time.

Since this task is turning into a requirement to fix up several layers of my buggy code, would it be fairer if I took it on?

I have the feeling that this would be quite a major project for me, given that the changes will require a detailed understanding of the interactions between multiple layers. If you have time to do it, I would appreciate that.
Find all posts by this user
04-07-2013, 11:15 AM
Post: #16
RE: Problem when action request doesn't have Content-Length header
(04-07-2013 11:07 AM)simoncn Wrote:  
(04-07-2013 10:07 AM)simonc Wrote:  Yes, this would constitute additional/undesirable allocation. Off the top of my head, options to avoid it include

Two passes through the event xml - the first discarding content but keeping track of the lengths written, allowing us to calculate the content length before sending the request body. There might be some neat implementations of this involving writing custom IWriter-derived classes.

I thought of this, but I wasn't sure if calling the methods that write the data would have any side-effects such as marking the updates as having been sent. Also, is it possible that the event data could change between the first write pass and the second write pass?

Good point. There is no guarantee that a property won't be updated while the content length calculation is underway, invalidating the calculated length. I'll need to think about whether more locking to enable this would be acceptable or if the approach needs to be abandoned...

Quote:
Quote:Writing this reminds me about a bug concerning dynamic allocation of PropertyWriterUpnp instances. It might be nice to tackle this at the same time.

Since this task is turning into a requirement to fix up several layers of my buggy code, would it be fairer if I took it on?

I have the feeling that this would be quite a major project for me, given that the changes will require a detailed understanding of the interactions between multiple layers. If you have time to do it, I would appreciate that.

No problem, I'll have a go at it, probably next week.
Find all posts by this user
30-07-2013, 03:35 PM (This post was last modified: 30-07-2013 03:35 PM by simoncn.)
Post: #17
RE: Problem when action request doesn't have Content-Length header
(04-07-2013 08:12 AM)simonc Wrote:  Oh, and the original bug noted in this thread should be fixed and on github now Smile

Unfortunately, this code has some problems that prevent ohNet from interoperating with the Simaudio MOON MiND. The details are:

1) The in-place dechunker relies on Srs filling its buffer before resetting its iOffset member. The Srx::Read method observes this requirement, but the Srx::ReadUntil method doesn't.

2) The length calculations in the new code in DviSessionUpnp::Post aren't correct:

2a) The 'len' variable is used for checking whether the chunked request data exceeds kMaxRequestBytes bytes, but the computed value fails to include chunk-terminating LF characters in the request data. This could cause a problem if the amount of chunked data is very slightly less than kMaxRequestBytes bytes.

2b) The 'len' variable is also used for copying request data chunks, but the computed value incorrectly includes chunking overhead bytes that aren't part of the data being copied. This could cause garbage characters to be appended to the dechunked request data.

I have fixed both these problems by changing the code so it computes separate length values ('bytes' and 'len') for 2a) and 2b).

3) The chunks are copied using memcpy(). This should be done using memmove() because the source and target regions can overlap.

I have fixed all these problems and I have also removed the incorrect code for reading request data from HTTP 1.0 control points until the connection is closed.

This fix has been tested by a Simaudio MOON MiND user. A patch is attached.


Attached File(s)
.zip  chunk.zip (Size: 1.44 KB / Downloads: 1)
Find all posts by this user
30-07-2013, 04:00 PM
Post: #18
RE: Problem when action request doesn't have Content-Length header
(30-07-2013 03:35 PM)simoncn Wrote:  Unfortunately, this code has some problems that prevent ohNet from interoperating with the Simaudio MOON MiND. The details are:

This fix has been tested by a Simaudio MOON MiND user. A patch is attached.

Thanks for spotting/fixing this. It'll take me a little time to remind myself how Srx is used but I'll try to get this integrated tomorrow.
Find all posts by this user
30-07-2013, 04:11 PM
Post: #19
RE: Problem when action request doesn't have Content-Length header
(30-07-2013 04:00 PM)simonc Wrote:  Thanks for spotting/fixing this. It'll take me a little time to remind myself how Srx is used but I'll try to get this integrated tomorrow.

The problem occurs when the data is read from the socket connection as two separate buffers (is this the correct terminology?)

First buffer:
length \r\n data \r\n

Second buffer:
0 \r\n \r\n

When Srx::ReadUntil reads the second buffer from the TcpSocket, it resets the iOffset member. This results in the data from the second buffer overlaying the start of the data from the first buffer.

My fix avoids resetting iOffset until the Srx is completely full. This means the data from the second buffer will be added to the end of the data from the first buffer instead of overlaying it.
Find all posts by this user
31-07-2013, 03:12 PM
Post: #20
RE: Problem when action request doesn't have Content-Length header
(30-07-2013 04:00 PM)simonc Wrote:  
(30-07-2013 03:35 PM)simoncn Wrote:  Unfortunately, this code has some problems that prevent ohNet from interoperating with the Simaudio MOON MiND. The details are:

This fix has been tested by a Simaudio MOON MiND user. A patch is attached.

Thanks for spotting/fixing this. It'll take me a little time to remind myself how Srx is used but I'll try to get this integrated tomorrow.

Now committed so will hopefully be available to you this evening.

Thanks again for diagnosing and fixing this - its much appreciated.
Find all posts by this user


Forum Jump: