tinygo-org / drivers

TinyGo drivers for sensors, displays, wireless adaptors, and other devices that use I2C, SPI, GPIO, ADC, and UART interfaces.
https://tinygo.org
BSD 3-Clause "New" or "Revised" License
623 stars 196 forks source link

wioterminal: webserver example fails with 'malformed HTTP request "G"' #476

Open scottfeldman opened 2 years ago

scottfeldman commented 2 years ago

In testing the wioterminal webserver example I bumped into a bug.

tinygo flash -target wioterminal -serial usb examples/rtl8720dn/webserver/main.go

The test is to repeatedly make an HTTP request for /hello. The webserver will return an HTTP response of "hello". The test runs for a few (10-100) iterations, returning the correct HTTP response of "hello", but then dies with:

malformed HTTP request "G"

The test is:

$ while true; do curl 192.168.1.60/hello; done,

where 192.168.1.60 is my wioterminal webserver address.

Attached is a minicom capture with debug=true and a few extra printfs I added to analyze. (Go to bottom to see error).

minicom.cap.malformed.log

Analysis

rtl87820dn/http.go:handleHTTP() is ignoring return result (-1) when calling Rpc_lwip_recv(). It looks like Rcp_lwip_recv() reads 1 byte into buf, so len(buf) > 0 and reading into buf is done. The one byte in buf is 'G'. (My guess is this is left over from a previous successful read request of "Get /hello HTTP/1.1"). But, the result value from the recv() call is -1, which handleHTTP() ignores. (I don't have documentation on the Rpc firmware to know the meanings on the result value, so some speculation on my part here as to what is a good result vs. bad result).

sago35 commented 2 years ago

I think I reproduced it in my environment. But I have not been able to check much yet. Perhaps, as you say, we need to correctly ignore the 0xFFFFFFFFFFFF shown in red.

image

Perhaps a return value check is needed here. And the return value could be implemented incorrectly as said in slack.

https://github.com/tinygo-org/drivers/blob/81bc1bcad1862f719556d3c7ff411c3f56b143dd/rtl8720dn/http.go#L69

I will check a little more.

scottfeldman commented 2 years ago

Hi, thanks for looking into this.

I have a rough fix that ran over night. The diff is below. Again, I don't have any documentation on the rpc firmware, so just making some educated guesses here.

This tries up to 10x to read into the buffer. If the read result == -1, we retry again. The worst case I saw was 4 retries. This only works if the whole http request is contained in the first successful read. If the read returns only a portion of the http request, we need to continue reading until end of the request (CR/LF/CR/LF) + body. but for my testing, it seems each http request is fetched with a single read.

I'm not sure why the second read into buf2 is there, or why four more reads happen before close, so I commented out that code and things still work.

diff --git a/rtl8720dn/http.go b/rtl8720dn/http.go
index 921ee9d..fdd9fd2 100644
--- a/rtl8720dn/http.go
+++ b/rtl8720dn/http.go
@@ -65,12 +65,20 @@ func (rtl *RTL8720DN) handleHTTP() error {
        }

        buf := make([]byte, 4096)
-       for {
-               _, err = rtl.Rpc_lwip_recv(socket, &buf, uint32(len(buf)), 8, 0)
+       for i := 0; i < 10; i++ {
+               buf_copy := buf
+               result, err := rtl.Rpc_lwip_recv(socket, &buf_copy, uint32(len(buf_copy)), 8, 0)
                if err != nil {
                        return nil
                }
-               if len(buf) > 0 {
+               if result == -1 {
+                       fmt.Printf("i=%d result=-1\r\n", i)
+                       time.Sleep(100 * time.Millisecond)
+                       continue
+               }
+               if len(buf_copy) > 0 {
+                       buf = buf_copy
+                       fmt.Printf("len(buf_copy)=%d len(buf)=%d\r\n", len(buf_copy), len(buf))
                        break
                }

@@ -82,6 +90,7 @@ func (rtl *RTL8720DN) handleHTTP() error {
                time.Sleep(100 * time.Millisecond)
        }

+       /*
        buf2 := make([]byte, 4096)
        result, err := rtl.Rpc_lwip_recv(socket, &buf2, uint32(len(buf2)), 8, 0)
        if err != nil {
@@ -98,6 +107,7 @@ func (rtl *RTL8720DN) handleHTTP() error {
        if result != 11 {
                return fmt.Errorf("Rpc_lwip_errno error")
        }
+       */

        b := bufio.NewReader(bytes.NewReader(buf))
        req, err := http.ReadRequest(b)
@@ -173,6 +183,7 @@ func (rtl *RTL8720DN) handleHTTP() error {
                }
        }

+       /*
        for i := 0; i < 4; i++ {
                buf := make([]byte, 4096)
                _, err = rtl.Rpc_lwip_recv(socket, &buf, uint32(len(buf)), 8, 0)
@@ -185,6 +196,7 @@ func (rtl *RTL8720DN) handleHTTP() error {
                        return nil
                }
        }
+       */

        _, err = rtl.Rpc_lwip_close(socket)
        if err != nil {
sago35 commented 2 years ago

I have created a tinygo driver in the following way

Much of the information can be found below. However, I think there is no detailed documentation.

scottfeldman commented 1 year ago

Just a note for now: this issue is fixed with the netdev branch.

scottfeldman commented 1 year ago

Fixed by #537.