The http client isn't binary-capable and is in fact a bit outdated

[wtarreau]

Hello!

Expected behavior

The callback called by http.get() should get the response body as it is received, not truncated at \0.

Actual behavior

The response body is handled as a C string and truncated at the first \0. This is very problematic because I expected to be able to use this method to update "lfs.img" and that's not possible since it's compressed and starts with 0x1F 8B 08 00 (hence only 3 bytes sent as the body).

I wanted to modify the code to measure the body length just after the strstr() looking for the double CRLF in app/http/httpclient.c, but I figured that there are at least two other bugs there, which are that the Transfer-encoding header is looked up anywhere in the response (including the body) and that it's looked up byte-for-byte (must have exactly one space after the colon) and without checking for non-comma, non-CRLF trailing chars. I could assign some time to work on fixing this as I really want to be able to use LFS and support updates. While looking around figuring where it was best to report all this, I found in the doc a reference to a client named esphttpclient which looks pretty similar and which more recently received a number of these fixes.

Thus I'm wondering if you're OK with me picking the fixes from esphttpclient and porting them to the httpclient there so that we can address this issue, or if instead you're aware of certain corner cases that ought to be avoided.

Test code

Provide a Minimal, Complete, and Verifiable example which will reproduce the problem.

-- add code here
> http.get("http://192.168.4.2:8080/lfs.img","range: bytes=0-1023\r\n",function(code,data,headers) print(code,headers["content-length"],#data) end)
> 206  1024     3

NodeMCU startup banner

You MUST include the firmware startup banner to describe the version you are using. We reserve the right to immediately close any bug that doesn't.

NodeMCU 3.0.0.0
  branch: master
  commit: 9cc80c481f59c89f4ffa14dd92b247e66d97543a
  release: 3.0-master_20190907 +4
  release DTS: 202002120559
  SSL: false
  build type: float
  LFS: 0x20000
  modules: adc,bit,dht,file,gpio,http,i2c,mqtt,net,node,ow,pwm,pwm2,rtctime,sntp,spi,tls,tmr,uart,u8g2,wifi
 build 2020-02-12 07:35 powered by Lua 5.1.4 on SDK 3.0.1-dev(fce08

Hardware

It's a D1-mini (ESP12F).

Thanks!
Willy

[pjsg]

The httpclient code is a bit of a mess -- it makes all sorts of assumptions. Fixing this binary issue would be useful. It might also be interesting to start again from scratch with a cleaner implementation.

[wtarreau]

I understand, but let's be fair, we have limited resources on such devices, and when you want to do HTTP correctly, it requires a huge amount of code to take care of everything. For most users, interoperability with reasonable implementations matters way more than perfect standards compliance. I don't see the current code as badly broken, it just has a few bugs and is inelegant, as any such code having evolved over time from something initially trivial. I think we'd rather just fix the corner cases and try to preserve resources.

[HHHartmann]

Please see #2405 and the associated PR. NOTE! the PR is untested, I just gathered the information form the repo given in the bug.

[wtarreau]

Ah excellent, thank you. I'll give that a try and will report back. And yes, at first glance it roughly does what I had in mind as well. It's missing the end-of-headers check for the lookup on te:chunked but that's a separate problem (which is definitely needed though).

[wtarreau]

Guys, I've just discovered HTTP_OTA.lua in lua_examples/lfs. It seems to be shorter, simpler, cleaner, and more maintainable than the C generic HTTP client. I think we could trivially derive it into a generic file download client with a callback on completion, which would, in the case of the HTTP_OTA code, just be the chunk of code surrounding node.flashreload(). It could then be used to flash an LFS image, to download a .lc file or even various config files in Lua or anything else.

Is there any interest in this ?

[marcelstoer]

I think we could trivially derive it into a generic file download client with a callback on completion

I first thought that @HHHartmann was working on that but then found #2968 and realized it's about an HTTP server there but a client here.

[wtarreau]

Hi Marcel, I managed to arrange it to use it do download `.lc` files, which is convenient, but I'm now realizing that it is less complete than the C version since it doesn't support HTTPS nor transfer-encoding. So I'm less convinced that it would make sense to use the Lua one as a replacement for the C version. I'll shortly send what I have after cleaning it up, and may get back to fixing the C version instead.

[HHHartmann]

@wtarreau have you tried the code from #2985? Does it work for you?

[wtarreau]

It's funny that you're asking about this today as I retried an HTTP transfer last evening and thought I should update the ticket! And no, I haven't tried this one, but I could try it this week-end if you want.

However looking at the patch I'm worried about this os_malloc() call. It might be related to how http_chunk_decode() is implemented but as a general rule there's no reason a chunked transfer would need a temporary copy. But I remember that this part of the code was quite convoluted and it might be possible that this is why we still need this allocation. I just hope we can never run out of memory because of this, as it's used mostly for updates, it would be a shame :-/

[HHHartmann]

If you could test the code that would be great.
the os_malloc just replaces the char chunked_decode_buffer[body_size];. So one allocates on the stack and the other on the heap. Both require the memory.
The module is not suitable for large replies, as it stores it in memory before returning the result. So probably not the right thing for fetching updates. At least not for LFS images which tend to be a bit larger.
But let's talk about the patch there and not here please.

[HHHartmann]

Fixed with #2985