coap_packet_get_payload() returns the wrong size

[efra-mx-aqua]

The function coap_packet_get_payload() returns the wrong size, it returns the number of bytes left for payload, no the actual payload size.
the current calculation is:

payload_len = cpkt->max_len - cpkt->hdr_len - cpkt->opt_len;

Which means that payload_len includes the unused bytes.

Snippet create packet:

#define MAX_COAP_MSG_LEN 768
#define COAP_UPLINK_TOKENLEN 2
	u8_t *payload, size_t plsize;
	enum coap_method code;
	const char uri[] = "/meta"
	u8_t *format = NULL;
	struct coap_packet request;
	int ret = 0;
	static u8_t buffer[MAX_COAP_MSG_LEN];
	u8_t *token = next_nonzero_coap_token(COAP_UPLINK_TOKENLEN);
	u8_t type = COAP_TYPE_NON_CON;
	u16_t id = coap_next_id();

	if (coap_packet_init(&request, buffer, sizeof buffer, 1,
			     type,
			     COAP_UPLINK_TOKENLEN,
			     token,
			     code, id) ||
	    coap_packet_append_option(&request, COAP_OPTION_URI_PATH,
				      uri, strlen(uri)) ||
	    (format && coap_packet_append_option(&request,
						 COAP_OPTION_CONTENT_FORMAT,
						 format, sizeof(*format)))) {
		LOG_ERR("CoAP packet wrapping error");
		return -EINVAL;
	}
	if (payload == 0 && plsize == 0)
		return 0;

	if (coap_packet_append_payload_marker(&request) ||
	    coap_packet_append_payload(&request, payload, plsize)) {
		LOG_ERR("CoAP payload error");
		return -EINVAL;
	}
       return 0;

Evaluating with the snippet from above:

/* Header length : (version + type + tkl) + code + id + [token] */
token = 2
hdr_len = 1 + 1 + 2 + 2 = 6
opt_len = 5 
payload_len = max_len - hdr_len - opt_len;
payload_len = 768 - 6 - 5 = 757;

The offset is moving with every append, therefore this actual pyload length can be calculated as follows:

payload_len = cpkt->offset - cpkt->hdr_len - cpkt->opt_len - 1;

This is what I get on my log:

15:11:39.771551 -- CoAP --
15:11:39.771581 version: 1
15:11:39.771611    code: 2
15:11:39.783170      id: 0xA785
15:11:39.783268     tkn: 0xDDB6
15:11:39.783305 tkn_len: 2
15:11:39.783333  pl_len: 757
15:11:39.783362    type: NON
15:11:39.783390  method: coap post
[00:10:27.547,943] <dbg> tx_manager: tx coap
15:11:39.783460 52 02 a7 85 dd b6 b4 6d |R......m
15:11:39.783492 65 74 61 ff a1 12 78 1a |eta...x.
15:11:39.783521 31 2e 33 2e 30 2d 72 63 |1.3.0-rc
15:11:39.783555 32 2d 36 66 66 30 62 38 |2-6ff0b8
15:11:39.783588 30 30 30 39 2d 64 69 72 |0009-dir
15:11:39.783616 74 79                   |ty      
15:11:39.783648 -- END CoAP --

This is what I would expect:

15:11:39.771551 -- CoAP --
15:11:39.771581 version: 1
15:11:39.771611    code: 2
15:11:39.783170      id: 0xA785
15:11:39.783268     tkn: 0xDDB6
15:11:39.783305 tkn_len: 2
15:11:39.783333  pl_len: 30
15:11:39.783362    type: NON
15:11:39.783390  method: coap post
[00:10:27.547,943] <dbg> tx_manager: tx coap
15:11:39.783460 52 02 a7 85 dd b6 b4 6d |R......m
15:11:39.783492 65 74 61 ff a1 12 78 1a |eta...x.
15:11:39.783521 31 2e 33 2e 30 2d 72 63 |1.3.0-rc
15:11:39.783555 32 2d 36 66 66 30 62 38 |2-6ff0b8
15:11:39.783588 30 30 30 39 2d 64 69 72 |0009-dir
15:11:39.783616 74 79                   |ty      
15:11:39.783648 -- END CoAP --

Environment:

• OS:Linux
• zephyr v1.14.1

[efra-mx-aqua]

@rlubos could you have a look at this?

[rlubos]

Sure, I'll analyze the issue and come back to you.

[efra-mx-aqua]

The coap specs says:

Following the header, token, and options, if any, comes the optional
payload. If present and of non-zero length, it is prefixed by a
fixed, one-byte Payload Marker (0xFF), which indicates the end of
options and the start of the payload. The payload data extends from
after the marker to the end of the UDP datagram, i.e., the Payload
Length is calculated from the datagram size.

[rlubos]

I think you're right, there seems to be some divergence in how particular struct coap_packet fields are handled, depending on whether the packet is created with Zephyr APIs, or parsed from the UDP packet. In the latter case, the calculation is correct, as the max_len is set to the actual datagram length, and the payload marker is actually included in the opt_len field (which is a bit counter-intuitive, to be honest). As the coap_packet_get_payload() was only used on the RX path so far, the issue was not identified.

I think we should unify how particular fields are handled in both cases, when a packet is created by the application, and when it's parsed from the received data. I think your proposal might be a good direction, however, it won't work in the latter case now, as parsing the packet does not set the offset field to the end of the payload.

I'll try to propose some cleanup in the near future, so that coap_packet_get_payload() works correctly in either case.

[efra-mx-aqua]

That is true actually, I had not noticed. I think that after the packet is parsed in coap_packet_parse() the offset shall be set to max_len. Since the packet has been validated in parse_option() an will return error in case the packet is malformed, what is remaining is the payload.

	cpkt->offset = cpkt->max_len + 1;

I will try to fix it here locally and once it works, I will try to submit a PR.

[rlubos]

cpkt->offset = cpkt->max_len + 1;

Why do you think we need +1 though? I think the offset should be equal to max_len - it indicates the packet is "full" - no more data will fit into the packet while indicating the correct packet length.

We'll also need more changes than merely setting the offset value, for the aforementioned payload marker, which is handled in a different way on TX/RX path. I think it should not be counted into opt_len when parsing as it's misleading IMHO.

I'd like to extend the current unit tests to cover this functionality better so we don't make changes blindly, I'll try to propose something today/tomorrow.

[efra-mx-aqua]

@rlubos I agree the +1 is not required.

I am currently trying to fix it, I will let you know what my findings are.

Thanks

[rlubos]

@efra-mx-aqua Please try this one: https://github.com/rlubos/zephyr/tree/coap/fix-get-payload I think this is the complete fix, I wanted to add some extra tests though before submitting a PR

[efra-mx-aqua]

I have something similar at the moment. I am just trying to do a proper fix for the coap module. Actually, I also found that opts_len is set always at least to 1, even when we there are no options.

[rlubos]

Actually, I also found that opts_len is set always at least to 1, even when we there are no options.

That's likely the Payload Marker which's incorrectly counted in the opt_len when packet is parsed. This should also be fixed on that branch.