net: buf: Make a `net_buf_add_str()` and other string handling

[josuah]

Introduction

Introduction of a net_buf based safe string handling library that is convenient to use while dealing with string-based network protocols.

[EDIT: This PR shows a possible implementation]

• lib: net_buf_simple: introduce string handling utilities #79167

Problem description

String-based network protocols require a fair amount of <string.h> handling, which requires extra care to avoid buffer overflows and other memory corruption. Boundary checks are currently done inline on every protocol handler.

Proposed change

The network library already contains safe wrappers around memory operations on buffers or integers.
The proposal is to add strings concatenation to the collection.

Detailed RFC

What kind of wrapper to build? What would be the underlying net_buf_... calls?

The ideal would be to have something to allow returning an error rather than hitting an assertion failure:

char *type = "KITE";

ret = net_buf_add_str(buf, "type: ");
if (ret < 0) {
        return ret;
}

ret = net_buf_add_str(buf, type);
if (ret < 0) {
        return ret;
}

ret = net_buf_add_str(buf, "\n");
if (ret < 0) {
        return ret;
}

return 0;

This becomes a bit heavyweight, so a string formatting wrapper could also be introduced:

char *type = "KITE";

ret = net_buf_add_fmt(buf, "type: %s\n", type);
if (ret < 0) {
        return ret;
}

return 0;

Then remains the problem of accessing the string: does net_buf_add_str/fmt() always add the trailing \0? This means removing it when concatenating like strncat() does, which could break code that interleaves binary and strings.

Proposed change (Detailed)

---

char *s = "with nul byte\0";
net_buf_add_mem(buf, s, strlen(s) + 1);
net_buf_add_mem(buf, s, strlen(s) + 1);

This leads to with nul byte\0with nul byte\0 instead of with nul bytewith nul byte\0
Not a viable API.

---

char *s = "with nul byte\0";
net_buf_add_mem(buf, s, strlen(s));
net_buf_add_mem(buf, s, strlen(s));

This leads to with nul bytewith nul byte instead of with nul bytewith nul byte\0
Not a viable API.

---

char *s = "with nul byte\0";
net_buf_add_mem(buf, s, strlen(s) + 1);
net_buf_remove_u8(buf); // remove the nul terminator to add another string
net_buf_add_mem(buf, s, strlen(s) + 1);

This works but could be misleading as data is removed from a buffer, so does not mix strings and

• net_buf_add_str(buf, s) check if there is a trailing \0 and removes it if so, then calls net_buf_add_mem(buf, s, strlen(s) + 1);

---

char *s = "with nul byte\0";
net_buf_add_mem(buf, s, strlen(s));
net_buf_add_mem(buf, s, strlen(s));
net_buf_add_u8(buf, '\0');

This leads to with nul bytewith nul byte\0 but easy to forget the terminator '\0'

• net_buff_add_str(buf, s) - calls net_buf_add_mem(buf, s, strlen(s)): no \0 added, fails if not enough room for the string + \0 terminator.
• net_buf_get_str(buf) - adds a trailing \0 if not already present and returns \s, assertion error upon failure.

---

It looks like the last option is the least ambiguous, but would require some care to not access buf->data directly as a \0 delmited string.

net_buf_add_fmt() would be:

int len;
len = vsnprintf(net_buf_tailroom(buf) + 1, NULL, ...);
if (len > net_buf_tailroom(buf)) {
       return -ENOMEM;
}
vsnprintf(net_buf_tailroom(buf), net_buf_add(buf, len), fmt, ...);
return 0;

Dependencies

By introducing new APIs on top of the existing ones, no API breakage is expected.

Concerns and Unresolved Questions

Is there something equivalent already in Zephyr?
I would appreciate feedback on the different trade-off and best compromise, and whether this approach is reasonable.

Alternatives

Introduction of extra check_compliance.py verification to try to spot most common pitfalls, and warn about the most misleading string.h functions such as strcat.

[jhedberg]

Thanks for the proposal @josuah. Generally this sounds like a useful addition to the net_buf API, so in that sense I'm in favor of it. Some thoughts:

• IMO the net_buf internal representation should always be binary and we just provide "to net_buf" and "from net_buf" APIs to go from an external representation to the internal one and vice-versa, i.e. basically the same as we do with existing byte-order specific integer helpers. We could consider diverging from this, but then I think we need to have some net_buf-internal flag to indicate what the internal encoding is.
• The string-based protocols you speak of, is the nul byte really transferred as well? I'd have assumed that it's only used for local representation of strings to indicate string termination but not something that actually gets transmitted as part of the protocol itself.
• If we keep the internal representation as-is (i.e. binary), then there wouldn't necessarily be a nul byte at all at the end, rather to get a nul-terminated string from a net_buf you'd need to provide your own buffer to copy into.
• The _fmt() helper sounds like a good idea, though we might still want the simpler API as well to save on code size when the printf format option isn't needed.
• Do we need considerations for UTF-8?
• Slight clarification to your examples: C string literals always have an implicit zero byte at the end, i.e. your examples with "...\0" actually have two zero bytes at the end (doesn't really make any practical difference to your API exploration proposals, though)

@jukkar @rlubos @tbursztyka (net_buf collaborators) FYI

[jukkar]

There is net_pkt API that we use extensively in networking which handles the data splitting to more than one net_buf (for both reading/writing data). If this proposal is to be used in for example in HTTP (I think you mentioned it in some discussion), then there needs to be support for strings in net_pkt API too.

I do not think we should add any additional \0 automatically as that is probably not expected in various protocol implementations.

[josuah]

@jhedberg

Good to be reminded! I follow and agree with each point mentioned. Some more answers/questions:

• The string-based protocols you speak of, is the nul byte really transferred as well? I'd have assumed that it's only used for local representation of strings to indicate string termination but not something that actually gets transmitted as part of the protocol itself.

Agreed! I was also hoping it to be usable in the string handling around the protocol. For instance, a net_buf received getting compared with strcmp(), or all internal string management.

Here for instance, a buffer whose len is 0 is being written to. It is probably fine as the author could make sure that for string message types there would always have at least one byte allocated:

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

Lines 626 to 633 in 0d118ec

	case LWM2M_RES_TYPE_STRING:
	if (len) {
	strncpy(data_ptr, value, len - 1);
	((char *)data_ptr)[len - 1] = '\0';
	} else {
	((char *)data_ptr)[0] = '\0';
	}
	break;

To verify that this is enforced, it is required to seek relatively far and also involves external callback exposed to this library's users:

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

Lines 890 to 898 in 0d118ec

	/* setup initial data elements */
	data_ptr = res_inst->data_ptr;
	data_len = res_inst->data_len;
	/* allow user to override data elements via callback */
	if (res->read_cb) {
	data_ptr = res->read_cb(obj_inst->obj_inst_id, res->res_id, res_inst->res_inst_id,
	&data_len);
	}

So that could also offer an internal/intermediate storage for string buffers used through protocols. Would that be overly invasive?

Do we need considerations for UTF-8?

UTF-8 can safely be contained in a nul-terminated char *string and is not affected by endianess, but maybe you meant validating UTF-8 strings? As far as I understand, ignoring UTF-8 would lead to safely storing incorrect strings in the worse case.

some net_buf-internal flag to indicate what the internal encoding is

This does not sound a requirement for correctly encoding UTF-8 strings, but could become required if wanting to handle UTF-16 indeed! I cannot name any protocol working with it except USB, which is already using net_buf.

Would it be fit for a different PR assuming "string" and "_str" refers to C strings as used in stdlib (agnostic of encoding)?
Glad to get some guidance on what these flags would be and do.

@jukkar:

data splitting to more than one net_buf (for both reading/writing data).

This reminds me of net_buf_append_bytes() which I just came across.

These are small wrappers, this could hopefully fit easily to net_pkt!

I do not think we should add any additional \0 automatically as that is probably not expected in various protocol implementations.

This is a good point!
One way to access a C string without actually modifying the buffer could be to check if there is enough room in buf->size, add a trailing \0, but not modify the buf->len. That way, buffers are safely terminated by \0 for the temporary time they are accessed as C strings, but not getting any data added.

[josuah]

Here is an initial implementation that illustrates the use-case:

• lib: net_buf_simple: introduce string handling utilities #79167

It does not yet handle:

• UTF-8 / UTF-16 / other encoding flags
• net_buf (only net_buf_simple)
• net_pkt (only net_buf_simple)
• push/pull/... as these are likely going to be net_buf_simple_..._mem() calls?

It does handle:

• Appending strings
• snprintf() formatting
• Accessing a net_buf as a C string without modifying the content.