net_buf: buf: assert on double free

[JordanYates]

Unreferencing a buffer that has already been freed should trigger an assertion as it indicates a critical logic error and potential security concern.

[jfischer-no]

I do not oppose it, but it looks a little weirder than before. Below, there will be another conditional double free check with a log and a return. This is followed by underflow check that would basically do the same, but it would not log anything, just return. Perhaps something like this would make it clearer:

diff --git a/lib/net_buf/buf.c b/lib/net_buf/buf.c
index 10fafc3a1c1..f489317c26a 100644
--- a/lib/net_buf/buf.c
+++ b/lib/net_buf/buf.c
@@ -447,21 +447,15 @@ void net_buf_unref(struct net_buf *buf)
                struct net_buf *frags = buf->frags;
                struct net_buf_pool *pool;
 
-               __ASSERT(buf->ref, "buf %p double free", buf);
-#if defined(CONFIG_NET_BUF_LOG)
                if (!buf->ref) {
-                       NET_BUF_ERR("%s():%d: buf %p double free", func, line,
-                                   buf);
+                       __ASSERT(0, "buf %p double free", buf);
+                       NET_BUF_ERR("buf %p double free", buf);
                        return;
                }
-#endif
+
                NET_BUF_DBG("buf %p ref %u pool_id %u frags %p", buf, buf->ref,
                            buf->pool_id, buf->frags);
 
-               if (--buf->ref > 0) {
-                       return;
-               }
-
+               buf->ref--;
                buf->data = NULL;
                buf->frags = NULL;

btw, with logging and asserts disabled, if (--buf->ref > 0) { would not prevent double free next 255 calls, right?

[jukkar]

Shouldn't we use proper atomic operations between decrement and checking of the ref counter?

[JordanYates]

I pulled in the #if defined condition to handle the CONFIG_ASSERT=n case.
The logging change would lose the function and line context from the logs.
The if (--buf->ref > 0) { suggestion is incorrect, that is the early exit for when the buffer still has references.

[jhedberg]

Shouldn't we use proper atomic operations between decrement and checking of the ref counter

We should, if it was an atomic_t, however it's a uint8_t (reasoning is to keep the net_buf struct as compact as possible, but I think it's fair to question this design decision.

[jfischer-no]

I pulled in the #if defined condition to handle the CONFIG_ASSERT=n case.

What is the point to keep #if defined then?

The if (--buf->ref > 0) { suggestion is incorrect,

Yes, it looks like I was partially wrong.

that is the early exit for when the buffer still has references.

That is the case with your latest changes. Before, it would still decrement and could cause another double free if the code took the same path multiple times.

[JordanYates]

What is the point to keep #if defined then?

Because the func and line variables are only function arguments if it is defined.

Before, it would still decrement and could cause another double free if the code took the same path multiple times.

Sure, but that was certainly not part of the API.

[jfischer-no]

@jhedberg Considering that there is a LOG_FUNC_NAME_PREFIX_ERR option that prefixes messages with the function name, does the net_buf code really need its own logging macros and all the #if defined(CONFIG_NET_BUF_LOG)?

[jhedberg]

@jhedberg Considering that there is a LOG_FUNC_NAME_PREFIX_ERR option that prefixes messages with the function name, does the net_buf code really need its own logging macros and all the #if defined(CONFIG_NET_BUF_LOG)?

I think the NET_BUF_LOG stuff is completely unnecessary. It predates the Zephyr logging subsystem, and these macros originally mapped to printk. I'd be happy to approve a PR that gets rid of it :)

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud