Skip to content

net_buf: buf: assert on double free #94311

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+6 −14
Open

net_buf: buf: assert on double free #94311

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cc07b48
tests: net_buf: buf: remove double free
JordanYates Aug 11, 2025
4531482
tests: udc: fix double free
JordanYates Aug 11, 2025
43fe3a2
net_buf: buf: remove custom assertion logic
JordanYates Aug 10, 2025
29b167b
net_buf: buf: assert on double free
JordanYates Aug 10, 2025
16bad38
net_buf: buf: return on double free
JordanYates Aug 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 4 additions & 8 deletions lib/net_buf/buf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,6 @@ LOG_MODULE_REGISTER(LOG_MODULE_NAME);
#define NET_BUF_INFO(fmt, ...)
#endif /* CONFIG_NET_BUF_LOG */

#define NET_BUF_ASSERT(cond, ...) __ASSERT(cond, "" __VA_ARGS__)

#if CONFIG_NET_BUF_WARN_ALLOC_INTERVAL > 0
#define WARN_ALLOC_INTERVAL K_SECONDS(CONFIG_NET_BUF_WARN_ALLOC_INTERVAL)
#else
Expand Down Expand Up @@ -331,9 +329,8 @@ struct net_buf *net_buf_alloc_len(struct net_buf_pool *pool, size_t size,
NET_BUF_DBG("allocated buf %p", buf);

if (size) {
#if __ASSERT_ON
size_t req_size = size;
#endif
__maybe_unused size_t req_size = size;

timeout = sys_timepoint_timeout(end);
buf->__buf = data_alloc(buf, &size, timeout);
if (!buf->__buf) {
Expand All @@ -343,9 +340,7 @@ struct net_buf *net_buf_alloc_len(struct net_buf_pool *pool, size_t size,
return NULL;
}

#if __ASSERT_ON
NET_BUF_ASSERT(req_size <= size);
#endif
__ASSERT_NO_MSG(req_size <= size);
} else {
buf->__buf = NULL;
}
Expand Down Expand Up @@ -452,6 +447,7 @@ 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@JordanYates JordanYates Aug 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@JordanYates JordanYates Aug 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 :)

#if defined(CONFIG_NET_BUF_LOG)
if (!buf->ref) {
NET_BUF_ERR("%s():%d: buf %p double free", func, line,
Expand Down
1 change: 0 additions & 1 deletion tests/drivers/udc/src/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,7 +576,6 @@ ZTEST(udc_driver_test, test_udc_enabled)
test_udc_ep_halt(dev, &ed_bulk_out);
test_udc_ep_disable(dev, &ed_bulk_out);
test_udc_ep_dequeue(dev, &ed_bulk_out);
test_udc_ep_buf_free(dev, buf);

err = udc_shutdown(dev);
zassert_equal(err, -EBUSY, "Not failed to shoot-down UDC driver");
Expand Down
1 change: 0 additions & 1 deletion tests/lib/net_buf/buf/prj.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
CONFIG_NET_BUF=y
CONFIG_NET_TEST=y
CONFIG_HEAP_MEM_POOL_SIZE=4096
#CONFIG_NET_BUF_LOG=y
#CONFIG_NET_BUF_LOG_LEVEL_DBG=y
Expand Down
2 changes: 0 additions & 2 deletions tests/lib/net_buf/buf/src/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,6 @@ ZTEST(net_buf_tests, test_net_buf_4)

if ((i % 2) && next) {
net_buf_frag_del(frag, next);
net_buf_unref(next);
removed++;
} else {
frag = next;
Expand All @@ -312,7 +311,6 @@ ZTEST(net_buf_tests, test_net_buf_4)
struct net_buf *frag2 = buf->frags;

net_buf_frag_del(buf, frag2);
net_buf_unref(frag2);
removed++;
}

Expand Down