Fix: in the WebSocket Client add-on, fix the base64 encoding

[Yves57]

When the number of bytes to encode is not a multiple of 3, the implementation of _nx_utility_base64_encode() uses an extra byte in the input buffer name that must equal to 0 (but this extra-byte must not be counted in name_size).

You can see a comment about that in this source code for example.

The bug causes the WebSocket handshake to fail randomly.

The fix has been double-checked with the following Go code as Base64 encoding reference:

func main() {
	digest := []byte{122, 200, 1, 65, 118, 43, 209, 42, 58, 252, 232, 253, 244, 242, 66, 67, 148, 135, 185, 105}
	key := base64.StdEncoding.EncodeToString(digest)
	fmt.Println(key)
}

[fdesbiens]

Thank you for the report @Yves57 , and especially for including the Go reference and the digest you tested with — that made this quick to check properly. Unfortunately, I cannot reproduce the behaviour the fix is based on, and I think the actual bug is somewhere else.

I extracted _nx_utility_base64_encode() verbatim from common/src/nx_utility.c into a harness, gave it an exactly-sized heap buffer so AddressSanitizer would trap any read past the end, and compared its output against an independent encoder for every input length from 1 to 40 bytes. Results: no ASan report at any size, output identical to the reference at every size, and — the decisive test — encoding the same input twice with name[name_size] set to 0x00 and then to 0xFF produces byte-identical output. The function does not consume that byte.

Your own vector confirms it. With digest = {122, 200, 1, 65, 118, 43, 209, 42, 58, 252, 232, 253, 244, 242, 66, 67, 148, 135, 185, 105} on a 20-byte exact-size buffer:

status       = 0
bytes_copied = 28
nx output    = esgBQXYr0So6/Oj99PJCQ5SHuWk=
go expected  = esgBQXYr0So6/Oj99PJCQ5SHuWk=
match        = YES        (no ASan over-read)

Reading the implementation confirms why. Only steps 1 and 2 of the encode loop look at name[i + 1], and both guard it with if ((i + 1) < input_name_size) (nx_utility.c:397 and :414), falling back to a zero-padded variant otherwise. Steps 0 and 3 only ever touch name[i], and the loop bound is such that i never reaches input_name_size while a read is pending. For the two sizes this add-on uses — 16 and 20 — the loop exits with i at 16 and 20 respectively, with no further read.

So guid[NX_WEBSOCKET_CLIENT_GUID_SIZE] = 0 and digest[NX_WEBSOCKET_ACCEPT_DIGEST_SIZE] = 0 are dead stores, and the two buffer enlargements are not needed.

Where the random failure probably comes from

This is the part worth pursuing, and I would rather redirect the PR than close it. _nx_websocket_client_connect_response_process() cannot handle a chained response packet, but it validates against the length of the whole chain:

• The parser is bounded entirely by the first packet: every loop tests against packet_ptr -> nx_packet_append_ptr (nx_websocket_client.c:991, :1005, :1017, :1037, :1049, :1060). It never follows nx_packet_next.
• The acceptance test at :1119 is offset != packet_ptr -> nx_packet_length. For a chained packet, nx_packet_length is the total across all packets in the chain.

So as soon as the 101 response arrives as a packet chain, offset only ever accounts for the first packet's bytes, the comparison at :1119 fails, and the handshake is rejected with NX_WEBSOCKET_INVALID_PACKET even though the response was perfectly valid. Whether a given response is chained depends on its size relative to the payload size of one packet, on MTU, and on how the server segments — which is exactly the kind of thing that varies from connection to connection and reads as "fails randomly".

Note that _nx_websocket_client_connect_response_check() does walk the chain looking for the CRLFCRLF terminator (:2652-2664), so a chained response reaches _nx_websocket_client_connect_response_process() rather than being rejected earlier. The two functions disagree about whether chains are supported.

If you can capture a failing handshake, the thing to check is whether nx_websocket_client_processing_packet -> nx_packet_next is non-NULL, and what offset versus nx_packet_length are at :1119. If that is what you are hitting, the fix is to make the parser walk the chain — or, if the intent really is that a handshake response must fit in one packet, to say so explicitly and fail with a clear status instead of via a length comparison.

While looking at that function I also noticed a separate latent bug in the two scan loops at :1005-1006 and :1060, described in a comment below. It is not the random failure, but it is wrong and it is in the same code path.

Process note

The PR is based on master; contributions should be based on dev, which then promotes to master. Happy to retarget it for you once we have settled on the direction.

Latent bug found nearby (separate from this PR)

You could bundle this fix in a refreshed PR or create a new PR to address it.

Anchor: addons/websocket/nx_websocket_client.c line 1060

Both header scan loops have an inverted compound condition:

while (((buffer_ptr + 1) < packet_ptr -> nx_packet_append_ptr) &&
       (*buffer_ptr != '\r') && (*(buffer_ptr + 1) != '\n'))

at :1005-1006 and again at :1060. The intent is "keep going while we have not found a CRLF pair", i.e. !(a == '\r' && b == '\n'), which by De Morgan is (*buffer_ptr != '\r') || (*(buffer_ptr + 1) != '\n'). Written with &&, the loop stops as soon as either holds, so it also stops on a bare LF that is not preceded by CR.

With well-formed CRLF line endings the two forms happen to agree, which is why this has not shown up. But against a server or proxy that emits bare LF line endings, the Sec-WebSocket-Accept value comes out one character short — 27 instead of 28 — and the check at :1119 rejects the handshake. That failure would be consistent per server rather than random, so it is probably not what you are seeing, but it is worth fixing while we are in here.

[fdesbiens]

The misconception behind thid PR is not yours — it is written down in the tree, at test/regression/mqtt_test/netx_mqtt_websocket_block_test.c:462-463:

/* Set the last extra byte of the digest to be zero, since the function _nx_utility_base64_encode will use this byte for calculation */
digest[20] = 0;

That claim is wrong for the same reasons as above, and since it is the thing that sent you down this path it should be removed so it stops misleading people. _nx_utility_base64_encode() would also benefit from an explicit note in its header block stating that it reads exactly name_size bytes.

Would you like to include that cleanup in this PR once the rest is reverted? It would turn the PR into a net improvement rather than a revert.

[fdesbiens]

This store has no effect on the result. _nx_utility_base64_encode() never reads name[name_size].

In the encode loop, name[i + 1] is only read inside step 1 (nx_utility.c:397) and step 2 (:414), and each is guarded by if ((i + 1) < input_name_size) with a zero-padded fallback in the else. Steps 0 (:390) and 3 (:431) read only name[i].

For the 20-byte digest here, name_size is adjusted to 27 output characters with 1 pad, and the loop terminates with i == 20 after step 2 takes its else branch — so index 20 is never dereferenced.

I verified this rather than relying on the reading. The function was extracted verbatim into a harness and given a 20-byte exact-size heap allocation under AddressSanitizer, using the digest from your PR description:

status       = 0
bytes_copied = 28
nx output    = esgBQXYr0So6/Oj99PJCQ5SHuWk=
go expected  = esgBQXYr0So6/Oj99PJCQ5SHuWk=
match        = YES

No ASan report, and the output already matches your Go reference without this store. Encoding the same input with name[20] set to 0xFF instead of 0 gives byte-identical output, so the byte genuinely does not participate.

Please drop this store and the + 1 on the digest declaration.

[fdesbiens]

Same as the comment on :1110. The GUID is 16 bytes, which is a multiple of neither 3 nor 4 but still does not cause an extra read: the encode loop ends with i == 16 after step 1 takes its guarded else branch, so guid[16] is never read.

I ran every length from 1 to 40 through the harness against an independent encoder, each on an exact-size heap buffer under ASan. Zero over-reads, zero output mismatches, and zero cases where the output changed when the byte at name[name_size] was flipped from 0x00 to 0xFF. Size 16 is included in that sweep.

So this store is dead and can be removed along with the + 1 in the header.

[fdesbiens]

Since the extra byte is never read, this enlarges NX_WEBSOCKET_CLIENT_STRUCT for nothing. In practice it will cost four bytes per client instance rather than one, because of the alignment of the pointer field that follows at :179.

NX_WEBSOCKET_CLIENT_STRUCT is a public type that applications allocate, so its size is part of the ABI — worth avoiding a change to it unless it buys something. Given ThreadX's footprint goals I would rather not pay this.

[fdesbiens]

Consistent with the store at :1111, so it should be reverted together with it. NX_WEBSOCKET_ACCEPT_DIGEST_SIZE is exactly the SHA-1 output length and _nx_sha1_digest_calculate() writes exactly that many bytes, so 20 is the correct size.

To be clear, the PR is internally consistent — it enlarges the array and writes to the new slot, so there is no out-of-bounds access here. The point is only that neither is needed.

[Yves57]

I don't reproduce the problem anymore. The proposition was not necessary. Closing the PR.
@fdesbiens Sorry for the noise!