Panic from write pump? - Githubissues

I was testing with a local docker setup and caused a panic with a /api/v3/device/send request.

{"caller":"manager.go:373","error":"read tcp 172.22.0.9:6200->172.22.0.15:51554: use of closed network connection","id":"mac:112233445566","level":"error","msg":"read error","ts":"2022-04-13T19:57:16.857210875Z"}
{"caller":"WRPHandler.go:71","code":504,"error":"That device has been closed","level":"error","method":"POST","msg":"Could not process device request","requestHeaders":{"Accept":["application/json"],"Accept-Encoding":["gzip, deflate, br"],"Authorization-Type":["Basic"],"Connection":["keep-alive"],"Content-Length":["230"],"Content-Type":["application/json"],"Postman-Token":["f05bb52f-35fe-4457-b220-c845909325bc"],"User-Agent":["PostmanRuntime/7.29.0"]},"requestURL":"/api/v2/device/send","span-id":"beb01e1063f81a59","trace-id":"be623103b17367917b95f5dc22616d34","ts":"2022-04-13T19:57:16.857221911Z"}
{"caller":"manager.go:294","closeError":null,"finalStatistics":"{\"bytesSent\": 0, \"messagesSent\": 0, \"bytesReceived\": 0, \"messagesReceived\": 0, \"duplications\": 0, \"connectedAt\": \"2022-04-13T19:56:28.268466022Z\", \"upTime\": \"48.5888231s\"}","id":"mac:112233445566","level":"error","msg":"Closed device connection","reason":"write-error","reasonError":null,"ts":"2022-04-13T19:57:16.858364351Z"}
{"caller":"panic.go:969","id":"mac:112233445566","level":"debug","msg":"writePump exiting","ts":"2022-04-13T19:57:16.858831033Z"}
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xa1d5b0]

goroutine 191 [running]:
github.com/ugorji/go/codec.encInBytes(...)
    /go/pkg/mod/github.com/ugorji/go/codec@v1.2.6/encode.go:1452
github.com/ugorji/go/codec.(*Encoder).ResetBytes(0xc0001ce3c8, 0x0)
    /go/pkg/mod/github.com/ugorji/go/codec@v1.2.6/encode.go:996 +0x30
github.com/xmidt-org/webpa-common/v2/device.(*manager).writePump(0xc00053a2c0, 0xc000147ef0, 0x1026e00, 0xc000121f20, 0xc00042f2c0, 0xc000421180)
    /go/pkg/mod/github.com/xmidt-org/webpa-common/v2@v2.0.1/device/manager.go:523 +0x75c
created by github.com/xmidt-org/webpa-common/v2/device.(*manager).Connect
    /go/pkg/mod/github.com/xmidt-org/webpa-common/v2@v2.0.1/device/manager.go:270 +0x113c

Looked into it and it looks like when we handle any /api/v3/device/send request that doesn't have a Content-Type: application/msgpack header, like Content-Type: application/json, it will trigger panic:

https://github.com/xmidt-org/webpa-common/blob/6f44bc6864bf17b3bbd6852fc3b2f6a74bb6b403/device/manager.go#L514-L524

 case envelope = <-d.messages: 
    var frameContents []byte 
    if envelope.request.Format == wrp.Msgpack && len(envelope.request.Contents) > 0 { 
        frameContents = envelope.request.Contents 
    } else { 
        // if the request was in a format other than Msgpack, or if the caller did not pass 
        // Contents, then do the encoding here. 
        encoder.ResetBytes(&frameContents) 
        writeError = encoder.Encode(envelope.request.Message) 
        encoder.ResetBytes(nil) 
    }

Where envelope.request.Format == wrp.Msgpack will evaluate to False and encoder.ResetBytes(nil) will kick off a panic due to an eventual nil dereferencing caused by ugorji's codec.encInBytes receiving that nil as its out

https://github.com/ugorji/go/blob/b4c725930670fc2d46721b17f4d6974c66fb50c1/codec/encode.go#L1451-L1452

func encInBytes(out *[]byte) (in []byte) {
    in = *out

I'm not entirely sure why encoder.ResetBytes(nil) was introduced, but it looks like it can be patched by simply removing it.

Tested the patch with the following:

curl -v --location --request POST 'localhost:6200/api/v3/device/send' \
--header 'Authorization: Basic ${AUTH}' \
--header 'Content-Type: application/json' \
--header 'Accept: application/json' \
--data-raw '{
"msg_type":3,
"content_type":"application/json",
"source":"dns:me",
"dest":"mac:112233445566",
"transaction_uuid":"1234567890",
"payload":"eyJjb21tYW5kIjoiR0VUIiwibmFtZXMiOlsiU29tZXRoaW5nIl19",
"partner_ids":["comcast"]
}'

// Output:

Note: Unnecessary use of -X or --request, POST is already inferred.
*   Trying ::1:6200...
* Connected to localhost (::1) port 6200 (#0)
> POST /api/v3/device/send HTTP/1.1
> Host: localhost:6200
> User-Agent: curl/7.77.0
> Authorization: Basic ${AUTH}
> Content-Type: application/json
> Accept: application/json
> Content-Length: 287
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< Content-Length: 648
< Content-Type: application/json
< X-Talaria-Build: 0.1.4
< X-Talaria-Flavor: mint
< X-Talaria-Region: east
< X-Talaria-Server: talaria
< X-Talaria-Start-Time: 21 Apr 22 16:23 UTC
< Date: Thu, 21 Apr 2022 16:42:49 GMT
< 
* Connection #0 to host localhost left intact
{"msg_type":3,"source":"mac:112233445566","dest":"dns:me","transaction_uuid":"1234567890","content_type":"application/octet-stream","metadata":{"partner-id":"comcast","hw-serial-number":"mock-rdkb-simulator","hw-manufacturer":"Example","hw-mac":"112233445566","hw-last-reboot-reason":"unknown","fw-name":"mock-rdkb-firmware","boot-time":"1650557221","webpa-last-reconnect-reason":"webpa_process_starts","webpa-protocol":"PARODUS-2.0-1.1.4-6-gad2d43b","hw-model":"aker-testing","webpa-interface-used":"eth0","webpa-uuid":"1234567-345456546"},"payload":"eyJzdGF0dXNDb2RlIjo1MzEsIm1lc3NhZ2UiOiJTZXJ2aWNlIFVuYXZhaWxhYmxlIn0=","partner_ids":["unknown"]}

xmidt-org / talaria

Panic from write pump? #226