Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The HTTP module has a bug that returns a binary with a byte of \0, which ends up with the actual data that's left behind #2405

Closed
etimes2008 opened this issue Jun 22, 2018 · 8 comments
Assignees

Comments

@etimes2008
Copy link

etimes2008 commented Jun 22, 2018

Expected behavior

Actual behavior

Test code

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

BUG:
The HTTP module has a bug that returns a binary with a byte of \0, which ends up with the actual data that's left behind

http.request("http://www.dayin.la/attachment/thumb/2018-01/23/user_89580_100x100.jpg", "GET", "Range: bytes=0-100\r\n", "",  function(code, data, headers)
    if (code < 0) then
      print("HTTP request failed")
    else
      print(code, #data, string.byte(data), crypto.toBase64(data), headers["content-length"])
    end
  end)

data size must be is 101,but size now is 4
d2ZyAGFmZApzZGY=206 4 255 /9j/4A== 101

NodeMCU version

Which branch are you on? If you know the Git revision then add it here as well.
nodemcu-firmware 2.2

Hardware

Describe which ESP8266 device you use and document any special hardware setup
required to reproduce the problem.

@etimes2008 etimes2008 changed the title nodemcu 里的http有个bug,返回二进制时里面有个\0的字节,它就结束了,其实后面还有的 nodemcu 里的http模块有个bug,返回二进制时里面有个\0的字节,它就结束了,其实后面还有的数据 Jun 22, 2018
@etimes2008 etimes2008 changed the title nodemcu 里的http模块有个bug,返回二进制时里面有个\0的字节,它就结束了,其实后面还有的数据 The HTTP module has a bug that returns a binary with a byte of \0, which ends up with the actual data that's left behind Jun 23, 2018
@anod221
Copy link

anod221 commented Aug 17, 2018

兄弟,我也遇到了这个问题,我修掉了这个bug。你需要的话在群里找我(唯笑不哭)。

translation:
Hey boy. I met this issue too. Now I have fixed it, you can ask for the firmware in the qq group if you need it. My nickname is 唯笑不哭

@marcelstoer
Copy link
Member

@anod221 can you please describe the fix or even better create a pull request here?

@anod221
Copy link

anod221 commented Aug 19, 2018

@marcelstoer I did these modification.
1 change type http_callback_t in file app/http/httpclient.h.
typedef void (* http_callback_t)(char * response_body, int http_status, char ** full_response_p, int body_size);
2 calculate the body size in function http_disconnect_callback in file app/http/httpclient.c.
3 modify the function http_callback in file app/module/http.c. use lua_pushlstring instead of lua_pushstring.

The original source is hard to modify. So I didn't process some code which may cause the memory leak.
This is why I didn't show the code. You can find the files here.

anod221/nodemcu-explore@1f2665d

@marcelstoer
Copy link
Member

@pjsg you were the last to work on the HTTP module, wonder what you make out of this. I'm certainly not in a position to process this.

@TerryE
Copy link
Collaborator

TerryE commented Aug 19, 2018

@marcelstoer, I had a quick scan of the code for the module http.c and the http library module. Lots of strlen() calls and lua_pushstring() not lua_pushlstring() calls so the module assumes Cstring responses. Will need work to support binary content. Some of Vowstar's early work.

In the meantime, those that need a workaround can do this in native Lua as per my HTTP_OTA.lua example. This also has the advantage that this algo is more stable for non-trivial files since it uses flow control to stop the sending server overrunning the ESP.

@stale
Copy link

stale bot commented Aug 14, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Aug 14, 2019
@HHHartmann
Copy link
Member

HHHartmann commented Aug 15, 2019

We should at least mention this in documentation. I didn't get if the modifications by @anod221 introduced the memory leak but we should still check to move it here.

@HHHartmann
Copy link
Member

Fixed with #2985

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants