How to report a library bug? (HttpClient)

Firmware Libraries
1

I found a bug on HttpClient. Not sure if this is the right place, so please let me know if there is another way.

The buffer variable is terminated correctly only if the response is as long as the buffer.

line 208: buffer[bufferPosition] = '\0';

In all other case previous responses might be part of the current request since buffer is never cleared.

My suggestion is to add

 memset(buffer, '\0', sizeof(buffer));

at the beginning of the HttpClient::request

Roge

2

As for the “HowTo” question, if you do find any issue and you have a github account, you could open a new issue (or search if it’s already reported) directly on the library repo.
Otherwise addressing this issue on the forum isn’t wrongeither.

As for your “bug” there are two ways of seeing it (not taking in consideration the HTTP side of the matter).
If buffer is considered to take a string of variable length, it’s perfectly fine to not clear the rest of the buffer, since strings are defined to be zero terminated and whatever follows after the terminating zero is not considered part of the string, so it can be left non-zero.
But if the buffer is actually considered an arbitrary byte buffer then clearing the rest might be a way to do it. But (again) if zero was a valid byte value in the buffer, youmight still be fooled into believing the cleared bytes were avtually transmitted, so you do definetly need to use some means of knowing the length of the transmitted array and use it in your code.

And all functions that hand you a variable length array, should give you back the length too - you have to use it tho’.

1 Like
3

I found the bug because I had a problem where previous http call responses where leaking on the current one. The point I was trying to make is that the buffer is an array of char that is terminated only in one case (line 208), which does not apply to the way I am using the HttpClient.

4

Here is another solution that makes it clearer what the problem is. Add:

buffer[bufferPosition+1] = '\0';

after line 206. In this way no matter how many characters are received, buffer is always correctly terminated.

5

I haven’t checked, but how is it with buffer overflow in this case?
Is there any code that prevents bufferPosition from becoming sizeof(buffer) - 1 and hence bufferPosition+1 exceeding the limits of buffer?

After having had a look into the (Web IDE) code, I guess there is a termination issue in the code.
But I would (for speed reasons) not terminate the string each time a new byte gets added to the buffer, but only after you drop out of the do { ... } while() (with consideration of bufferPosition vs. sizeof(buffer)).

But if you have a look at the version on github, you’ll see that issue has already been taken care of, it just didn’t get pushed onto Web IDE yet. Maybe pinging @nmattisson for that to happen might cure your problem :wink:
But as mentioned above in the unlucky event of dropping out of while() {...} when bufferPosition == sizeof(buffer) the current version on github might cause a buffer overrun.

while (client.available()) 
{
   ...
  // Check that received character fits in buffer before storing.
  if (bufferPosition < sizeof(buffer)-1) 
    buffer[bufferPosition] = c;
  else if ((bufferPosition == sizeof(buffer)-1)) 
  {
    buffer[bufferPosition] = '\0'; // Null-terminate buffer
    client.stop();
    error = true;
  }
  bufferPosition++;  // <-- after (bufferPosition == sizeof(buffer)-1) 
                     //     causes bufferPosition == sizeof(buffer)
}
buffer[bufferPosition] = '\0'; // <-- hence this might overrun the buffer limits
6

Sorry if I was not clear. This is not a buffer overflow. It is a matter of past requests leaking in current ones.
Let’s say you have two requests first one returning “Ret: 0.1” and the following request returning “Ret: 1”.

Now the response.body after the 2nd request will be “Ret: 1.1” since buffer was never terminated correclty.

Let me know if this clarify the issue.
Roge

7

You were clear enough and I understood just right.

As I said, that bug you see in the Particle Web IDE version of the HttpClient library was already taken care of (= it gets terminated propperly now) in the github version (see link provided).
But, that fix may produce a buffer overrun in case of an long HTTP packet (>= sizeof(buffer) - 1), due to reasons mentioned above.

8

Thanks for clarifying. Now I understand.

I think both of my solutions will not run into a buffer overflow. But you are correct that it adds a termination for every character received. The buffer overflow should not be a problem since bufferPosition is less than the sizeof(buffer)-1.

As for the version on github, I think it should be possible to just check for the overflow, like:

if (bufferPosition<sizeof(buffer)-1)  buffer[bufferPosition] = '\0';

This should not run into any buffer overflow.