Fix wrong chunk length

[biglben]

In some rare cases there is a small subsequent frame which is not read and transceive aborts with wrong chunk length

I have mcuboot (zephyr) with the option CONFIG_SINGLE_APPLICATION_SLOT=y and use the serial recovery mode. There is an image on the controller with the version 3.7.99.782043700. I wanted to get the list of images and it failed with wrong chunk length

To be honest I don't completely understand the code. I just saw that there is only the first frame read and the second not. Here

mcumgr-client/src/transfer.rs

Line 171 in 3d910e0

if len != decoded.len() - 2 {

the 2 bytes to encode the length are subtracted and here

mcumgr-client/src/transfer.rs

Line 160 in 3d910e0

if decoded.len() >= expected_len {

not.
I subtracted this 2 bytes too and it works now. I tested list, upload and reset with the fix and had no issues.

[Frank-Buss]

I'm not sure it this really fixes it. But this PR introduces a possible new problem: imagine only one byte of a chunk is transferred, and then a newline. Since decoded.len() is usize, this would underflow and the read would terminate. But should be easy to fix.

And could you post the transferred data with the Go mcumgr program in debug mode, in a gist, which demonstrates your original problem? Then it is easier for me to check it.

[biglben]

I'm not sure it this really fixes it. But this PR introduces a possible new problem: imagine only one byte of a chunk is transferred, and then a newline. Since decoded.len() is usize, this would underflow and the read would terminate. But should be easy to fix.

I am not sure if thats a problem. BigEndian::read_u16 will panic if len is smaller than 2. But it would be invalid data anyway.

And could you post the transferred data with the Go mcumgr program in debug mode, in a gist, which demonstrates your original problem? Then it is easier for me to check it.

I don't have the same image anymore but 3.7.99.784070009 causes the same error

go client

root@linux:~# /tmp/mcumgr -c mcu -l debug image list
DEBU[2024-07-09 08:04:29.099] Using connection profile: name=mcu type=serial connstring=dev=/dev/ttymxc2,baud=230400 
DEBU[2024-07-09 08:04:29.1] {add-nmp-listener} [serial_sesn.go:213] seq=66 
DEBU[2024-07-09 08:04:29.1] Encoded &{NmpBase:{hdr:{Op:0 Flags:0 Len:0 Group:1 Seq:66 Id:0}}} to:
00000000  a0                                                |.| 
DEBU[2024-07-09 08:04:29.1] Encoded:
00000000  00 00 00 01 00 01 42 00  a0                       |......B..| 
DEBU[2024-07-09 08:04:29.1] Tx NMP request: 00000000  00 00 00 01 00 01 42 00  a0                       |......B..| 
DEBU[2024-07-09 08:04:29.1] Base64 encoding request:
00000000  00 00 00 01 00 01 42 00  a0                       |......B..| 
DEBU[2024-07-09 08:04:29.1] Tx serial
00000000  06 09                                             |..| 
DEBU[2024-07-09 08:04:29.1] Tx serial
00000000  41 41 73 41 41 41 41 42  41 41 46 43 41 4b 44 31  |AAsAAAABAAFCAKD1|
00000010  4d 77 3d 3d                                       |Mw==| 
DEBU[2024-07-09 08:04:29.1] Tx serial
00000000  0a                                                |.| 
DEBU[2024-07-09 08:04:29.525] Rx serial:
00000000  06 09 41 46 30 42 41 41  42 54 41 41 46 43 41 4c  |..AF0BAABTAAFCAL|
00000010  39 6d 61 57 31 68 5a 32  56 7a 6e 37 39 6b 63 32  |9maW1hZ2Vzn79kc2|
00000020  78 76 64 41 42 6b 61 47  46 7a 61 46 67 67 4b 6d  |xvdABkaGFzaFggKm|
00000030  53 72 70 78 6a 59 4e 52  6c 6c 7a 68 74 2b 42 45  |SrpxjYNRllzht+BE|
00000040  30 56 39 34 73 49 64 6e  50 43 59 69 30 50 46 6a  |0V94sIdnPCYi0PFj|
00000050  30 78 58 4b 35 39 46 64  46 6e 64 6d 56 79 63 32  |0xXK59FdFndmVyc2|
00000060  6c 76 62 6e 41 7a 4c 6a  63 75 4f 54 6b 75 4e 7a  |lvbnAzLjcuOTkuNz|
00000070  67 30 4d 44 63 77 4d 44  41 35 2f 2f 2f 2f        |g0MDcwMDA5////| 
DEBU[2024-07-09 08:04:29.526] Rx serial:
00000000  04 14 2f 41 41 3d                                 |../AA=| 
DEBU[2024-07-09 08:04:29.526] Decoded input:
00000000  01 00 00 53 00 01 42 00  bf 66 69 6d 61 67 65 73  |...S..B..fimages|
00000010  9f bf 64 73 6c 6f 74 00  64 68 61 73 68 58 20 2a  |..dslot.dhashX *|
00000020  64 ab a7 18 d8 35 19 65  ce 1b 7e 04 4d 15 f7 8b  |d....5.e..~.M...|
00000030  08 76 73 c2 62 2d 0f 16  3d 31 5c ae 7d 15 d1 67  |.vs.b-..=1\.}..g|
00000040  76 65 72 73 69 6f 6e 70  33 2e 37 2e 39 39 2e 37  |versionp3.7.99.7|
00000050  38 34 30 37 30 30 30 39  ff ff ff                 |84070009...| 
DEBU[2024-07-09 08:04:29.526] rx nmp response: 00000000  01 00 00 53 00 01 42 00  bf 66 69 6d 61 67 65 73  |...S..B..fimages|
00000010  9f bf 64 73 6c 6f 74 00  64 68 61 73 68 58 20 2a  |..dslot.dhashX *|
00000020  64 ab a7 18 d8 35 19 65  ce 1b 7e 04 4d 15 f7 8b  |d....5.e..~.M...|
00000030  08 76 73 c2 62 2d 0f 16  3d 31 5c ae 7d 15 d1 67  |.vs.b-..=1\.}..g|
00000040  76 65 72 73 69 6f 6e 70  33 2e 37 2e 39 39 2e 37  |versionp3.7.99.7|
00000050  38 34 30 37 30 30 30 39  ff ff ff                 |84070009...| 
DEBU[2024-07-09 08:04:29.527] Received nmp rsp: &{NmpBase:{hdr:{Op:1 Flags:0 Len:83 Group:1 Seq:66 Id:0}} Rc:0 Images:[{NmpBase:{hdr:{Op:0 Flags:0 Len:0 Group:0 Seq:0 Id:0}} Image:0 Slot:0 Version:3.7.99.784070009 Hash:[42 100 171 167 24 216 53 25 101 206 27 126 4 77 21 247 139 8 118 115 194 98 45 15 22 61 49 92 174 125 21 209] Bootable:false Pending:false Confirmed:false Active:false Permanent:false}] SplitStatus:N/A} 
DEBU[2024-07-09 08:04:29.527] {remove-nmp-listener} [serial_sesn.go:213] seq=66 
Images:
 image=0 slot=0
    version: 3.7.99.784070009
    bootable: false
    flags: 
    hash: 2a64aba718d8351965ce1b7e044d15f78b087673c2622d0f163d315cae7d15d1
Split status: N/A (0)

Unmodified mcumgr-client

root@linux:~# /tmp/mcumgr-client -d /dev/ttymxc2 -b 230400 -t 10 -v list
mcumgr-client 0.0.4, Copyright © 2024 Vouch.io LLC

06:04:36 [INFO] send image list request
06:04:36 [DEBUG] (1) mcumgr_client::transfer: request header: NmpHdr { op: Read, flags: 0, len: 1, group: Image, seq: 180, id: 0 }
06:04:36 [DEBUG] (1) mcumgr_client::transfer: serialized: 000000010001b400a0
06:04:36 [DEBUG] (1) mcumgr_client::transfer: encoded with packet length and checksum: 000b000000010001b400a0a4c1
06:04:36 [DEBUG] (1) mcumgr_client::transfer: encoded: AAsAAAABAAG0AKCkwQ==
06:04:36 [DEBUG] (1) mcumgr_client::transfer: expected length: 93
06:04:36 [DEBUG] (1) mcumgr_client::transfer: result string: AF0BAABTAAG0AL9maW1hZ2Vzn79kc2xvdABkaGFzaFggKmSrpxjYNRllzht+BE0V94sIdnPCYi0PFj0xXK59FdFndmVyc2lvbnAzLjcuOTkuNzg0MDcwMDA5////
06:04:36 [ERROR] Error: wrong chunk length

Modified mcumgr-client

root@linux:~# mcumgr-client -d /dev/ttymxc2 -b 230400 -t 10 -v list
mcumgr-client 0.0.4, Copyright © 2024 Vouch.io LLC

06:04:43 [INFO] send image list request
06:04:43 [DEBUG] (1) mcumgr_client::transfer: request header: NmpHdr { op: Read, flags: 0, len: 1, group: Image, seq: 159, id: 0 }
06:04:43 [DEBUG] (1) mcumgr_client::transfer: serialized: 0000000100019f00a0
06:04:43 [DEBUG] (1) mcumgr_client::transfer: encoded with packet length and checksum: 000b0000000100019f00a0d2f6
06:04:43 [DEBUG] (1) mcumgr_client::transfer: encoded: AAsAAAABAAGfAKDS9g==
06:04:44 [DEBUG] (1) mcumgr_client::transfer: expected length: 93
06:04:44 [DEBUG] (1) mcumgr_client::transfer: result string: AF0BAABTAAGfAL9maW1hZ2Vzn79kc2xvdABkaGFzaFggKmSrpxjYNRllzht+BE0V94sIdnPCYi0PFj0xXK59FdFndmVyc2lvbnAzLjcuOTkuNzg0MDcwMDA5////6V4=
06:04:44 [DEBUG] (1) mcumgr_client::transfer: response header: NmpHdr { op: ReadRsp, flags: 0, len: 83, group: Image, seq: 159, id: 0 }
06:04:44 [DEBUG] (1) mcumgr_client::transfer: cbor: bf66696d616765739fbf64736c6f7400646861736858202a64aba718d8351965ce1b7e044d15f78b087673c2622d0f163d315cae7d15d16776657273696f6e70332e372e39392e373834303730303039ffffff
06:04:44 [INFO] response: {
  "images": [
    {
      "hash": [
        42,
        100,
        171,
        167,
        24,
        216,
        53,
        25,
        101,
        206,
        27,
        126,
        4,
        77,
        21,
        247,
        139,
        8,
        118,
        115,
        194,
        98,
        45,
        15,
        22,
        61,
        49,
        92,
        174,
        125,
        21,
        209
      ],
      "slot": 0,
      "version": "3.7.99.784070009"
    }
  ]
}

[Frank-Buss]

Thanks, you are right, the panic should be safe, wouldn't happen with valid data anyway. You patch is right, the length of the decoded data needs to be added (so -2 of your patch reads the 2 more bytes). I guess it worked so far only by change, because the base 64 decoding failed before, because it didn't use general_purpose::NO_PAD, pretty tricky.