net: lib: coap: client: Invalid path pointer

[pdgendt]

Describe the bug

Using the CoAP client to make a request using a path located for example on the stack, it is later used to evaluate matching requests.

Consider the following snippets:

static int foo_get(uint16_t id)
{
	/* Construct my dynamic path */
	char path[sizeof("foo/xxxxx")];
	snprintf(path, sizeof(path), "foo/%d", id);

	/* Do request */
	struct coap_client_request req = {
		.method = COAP_METHOD_GET,
		.path = path, /* <--------- A stack buffer */
		.cb = foo_cb,
	};
	/* Doing the request is fine because the CoAP packet is constructed
	 * and the path is converted to CoAP options */
	return coap_client_req(&client, 0, &dst_address, &req, NULL));
}

If at any point in the lifetime of the request a call is done to coap_client_cancel_request or coap_client_cancel_requests it will compare an internally stored copy of the pointer here:

zephyr/subsys/net/lib/coap/coap_client.c

Lines 1064 to 1066 in 491498a

	if (a->path && b->path && strcmp(a->path, b->path) != 0) {
	return false;
	}

Resulting in calling strcmp with an invalid pointer.

Regression

• This is a regression.

Steps to reproduce

No response

Relevant log output

Impact

Functional Limitation – Some features not working as expected, but system usable.

Environment

No response

Additional Context

No response

[rlubos]

After spending some time with the CoAP client I think the only valid assumption is that all data provided within struct coap_client_request should remain valid throughout the request lifetime. The issue is, the client library creates a soft copy of the request structure internally, which is then used for retransmissions or block transfers. The pointer issue is not only the case for the path pointer, but also for payload or custom options.

[pdgendt]

That greatly limits the use of the CoAP client IMO.

[rlubos]

Just thinking out loud what can be done here, we can try to fix that but at the cost of increasing the library memory requirements. Looking at the current struct:

/**
 * @brief Representation of a CoAP client request.
 */
struct coap_client_request {
	enum coap_method method;                  /**< Method of the request */
	bool confirmable;                         /**< CoAP Confirmable/Non-confirmable message */
	const char *path;                         /**< Path of the requested resource */
	enum coap_content_format fmt;             /**< Content format to be used */
	const uint8_t *payload;                   /**< User allocated buffer for send request */
	size_t len;                               /**< Length of the payload */
	coap_client_response_cb_t cb;             /**< Callback when response received */
	const struct coap_client_option *options; /**< Extra options to be added to request */
	uint8_t num_options;                      /**< Number of extra options */
	void *user_data;                          /**< User provided context */
};

path and options case could possibly be fixed easily by embedding path/options buffers directly into the request struct with some configurable upper limits. But payload pointer is trickier (having block transfer in mind), turning that into a buffer doesn't seem reasonable. But perhaps we could limit the requirement to keep payload pointer valid throughout the transaction for block transfer cases only if we refactor the retransmission logic.

Currently there's a single TX buffer within the struct coap_client context structure (

zephyr/include/zephyr/net/coap_client.h

Line 131 in e5838ff

uint8_t send_buf[MAX_COAP_MSG_LEN];

), which can be reused freely by various request contexts. That's why it's needed to rebuild the message for each retransmission, as two parallel requests could interfere with each other. If we move the TX buffer to struct coap_client_internal_request, the library will need more RAM, but we could just retransmit the originally created packet w/o a need to rebuild. That way, for simple cases (non-block uploads), the payload would just be copied inside the packet and the pointer won't need to remain valid for retransmissions.

So to recap:

1. Replace path pointer with path buffer, which will be copied into internal request context.
2. Same for custom options, with the maximum number of options configurable in Kconfig.
3. Multiply TX buffers, one for each internal request context. That way retransmissions won't require access to the payload pointer.

That way we could limit the requirement to keep the pointers valid to payload pointers provided for block transfer only, which would be needed to be documented well. Or #97107 could be used instead for payload.

Thoughts?

[rlubos]

@SeppoTakalo For visibility, FYI

[rlubos]

I've opened #97328 with suggested changes, let's discuss there if we want to rework the library that way.