BUG in nrf_ringbuf in SDK 16.0.0 and SDK 17.0.2

[Bruno Randolf]

nrf_ringbuf has a bug which happens when mixing nrf_ringbuf_cpy_put() with nrf_ringbuf_alloc() and nrf_ringbuf_put(). After copying data with nrf_ringbuf_cpy_put() a later alloc will overwrite the same buffer. See the following example code:

NRF_RINGBUF_DEF(ring, 128);

nrf_ringbuf_init(&ring);

size_t len;

uint8_t buf[]

= {0xa1, 0xa2, 0xa3, 0xa4, 0xa5, 0xa6, 0xa7, 0xa8, 0xa9, 0xaa};

uint8_t buf2[] = {0xb1, 0xb2, 0xb3, 0xb4, 0xb5};

len = 10;

nrf_ringbuf_cpy_put(&ring, buf, &len);

uint8_t* pdat;

nrf_ringbuf_alloc(&ring, &pdat, &len, true);

memcpy(pdat, buf2, 5);

nrf_ringbuf_put(&ring, 5);

len = 15;

nrf_ringbuf_get(&ring, &pdat, &len, true);

LOG_INF("got %d: %x %x %x %x %x %x %x %x %x %x %x %x %x %x %x", len,

pdat[0], pdat[1], pdat[2], pdat[3], pdat[4], pdat[5], pdat[6],

pdat[7], pdat[8], pdat[9], pdat[10], pdat[11], pdat[12], pdat[13],

pdat[14]);

I would expect the output to be:

got 15: a1 a2 a3 a4 a5 a6 a7 a8 a9 aa b1 b2 b3 b4 b5

But instead it overwrites the first bytes:

got 15: b1 b2 b3 b4 b5 a6 a7 a8 a9 aa 0 0 0 0 0

Adding the following to nrf_ringbuf.c line 138 in nrf_ringbuf_cpy_put() fixes it:

p_ringbuf->p_cb->tmp_wr_idx = p_ringbuf->p_cb->wr_idx;

Also I suspect there may be a similar bug when getting with nrf_ringbuf_cpy_get() vs nrf_ringbuf_get() and nrf_ringbuf_free() but i have not verified or used it.

The bug is present in SDK 16 and 17.

Please fix, I find it very disturbing that I can't trust even such a simple library function within the official SDK!

[Einar Thorsrud]

Hi,

Thank you for reporting and suggesting a fix. I see the issue on my side. I have created an internal bug report so that it can be fixed in a future release.

[Shahar]

Any idea on how to fix it before the next SDK release?

As of now I assume the only workaround is to not mix...

[Einar Thorsrud]

This diff shows a fix:

Fullscreen nrf_ringbuf.c.diff Download

diff --git a/components/libraries/ringbuf/nrf_ringbuf.c b/components/libraries/ringbuf/nrf_ringbuf.c
index 24cca79..0f93dc2 100644
--- a/components/libraries/ringbuf/nrf_ringbuf.c
+++ b/components/libraries/ringbuf/nrf_ringbuf.c
@@ -135,6 +135,7 @@ ret_code_t nrf_ringbuf_cpy_put(nrf_ringbuf_t const * p_ringbuf,
     }
     memcpy(&p_ringbuf->p_buffer[masked_wr_idx], p_data, length);
     p_ringbuf->p_cb->wr_idx += *p_length;
+    p_ringbuf->p_cb->tmp_wr_idx = p_ringbuf->p_cb->wr_idx;
 
     UNUSED_RETURN_VALUE(nrf_atomic_flag_clear(&p_ringbuf->p_cb->wr_flag));
 
@@ -215,6 +216,7 @@ ret_code_t nrf_ringbuf_cpy_get(nrf_ringbuf_t const * p_ringbuf,
     }
     memcpy(p_data, &p_ringbuf->p_buffer[masked_rd_idx], length);
     p_ringbuf->p_cb->rd_idx += *p_length;
+    p_ringbuf->p_cb->tmp_rd_idx = p_ringbuf->p_cb->rd_idx;
 
     UNUSED_RETURN_VALUE(nrf_atomic_flag_clear(&p_ringbuf->p_cb->rd_flag));