LWM2M writing too long strings trigger post_write_cb with previously written value

[petterssonandreas]

Describe the bug
You get no error or warning if you try to write a too long string from a LWM2M server, instead the code will silently use the last written value (that had a good length), and trigger post_write_cb and the like with that value, effectily making it look like you wrote the same value once again.

To Reproduce
We can define an IPSO object with a field of type String, and set its resource to some static char array, and using the size of that array as the size of the resource. We can also register eg. post_write callback for this field.

If we then write that field from a LWM2M server with a string that is longer than the resource array, we don't get an error. Instead, the resource array is simply not overwritten with the new value, but keeps the old value, and then carries on processing the write as if that value is the newly written value. This will then trigger eg. post_write callback with the previous value, making it look like we write the same value again.

Expected behavior
I would expect the lwm2m engine to throw an error if the string is too long, not allowing the write, and stopping the processing of it.

zephyr/subsys/net/lib/lwm2m/lwm2m_rw_oma_tlv.c

Lines 632 to 634 in 875c75c

	if (buflen <= tlv.length) {
	/* TODO: Generate error? */
	return 0;

The above code has a TODO regarding this, and does return 0, indicating that nothing was written. But lwm2m engine does not take this into account:

zephyr/subsys/net/lib/lwm2m/lwm2m_engine.c

Lines 2555 to 2558 in f2ca6a8

	case LWM2M_RES_TYPE_STRING:
	engine_get_string(&msg->in, write_buf, write_buf_len);
	len = strlen((char *)write_buf);
	break;

ignoring the return from the engine_get_string, and just assuming the write_buf was written to.

This would be easy to fix, changing the code above to:

case LWM2M_RES_TYPE_STRING:
	if (engine_get_string(&msg->in, write_buf, write_buf_len) <= 0) {
		/* -EEXIST will generate Bad Request LWM2M response. */
		return -EEXIST;
	}
	len = strlen((char *)write_buf);
	break;

Using either == 0 or <= 0 depending on what we want to cover.

Impact
I have found no other way to actually check and error on the length of a string. I have tried adding a longer validate buffer, and that works in some sense, but basically only delaying the problem, since the exakt same problem will occur when the written string is longer than the validate buffer.

I can design other guards for this, or adding more fields for the length etc, but it's not pretty or clean. I can also design my code to not care about the multiple writes of the same value, but the problem is really that the user of the server thinks that the new, too long, value was actually written.

Environment (please complete the following information):

• OS: Linux (Ubuntu 20.04)
• Toolchain: gnuarmemb
• Zephyr version: Tested on tag v2.7.0-ncs1, but also check Zephyr main and saw no fix to this.
• LWM2M server: Eclipse Leshan, but with our own modifications

[rlubos]

Hi @petterssonandreas, thanks for the report.

I think this issue should already be fixed in my PR (#41744), where I've reworked error handling in the LwM2M content writers in general. Could you have a look?

[petterssonandreas]

Hi @rlubos,

Great that you have made a PR for this! I took a quick look, and left some comments. I am not too familiar with the code so didn't really look at everything, but I tried to look at the parts that this issue covers.

And yes, I think this is mostly covered by your PR, although some of my comments point out code that could be potentially harmful (or at least still annoying to users), the ones where we read/write the write_buf without checking the return code.