Segmentation Fault in UDP Source Block

[pamin98]

What happened?

The UDP Source block produces a segmentation fault when any UDP packets are received on the specified port.
GDB Logs and steps to reproduce are included in the ticket.

System Information

OS: Ubuntu 20.04.02 LTS
GR Installation Method: native distro packages - apt

GNU Radio Version

3.10 (maint-3.10)

Specific Version

3.10.5.0

Steps to Reproduce the Problem

Create a chain consisting of a UDP Source, a Throttle block and a File Sink. Relevant configurations:
UDP Source
Input Type: byte
Port: 30000
Header: None (or any other)
UDP Packet data size: 1472
Notify missed frames: No
Src 0s: No
IPv6: No
Vector Len: 1

Throttle
Type: byte
The rest as default

File Sink:
File: /home/user/somefile.bin
Type: byte
The rest as default.

---

Execute the flow graph and send some UDP packets to port 30000.
The execution crashes with return code 11.

Relevant log output

GDB output
----------
(...)
Thread 23 "udp_source1" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffc621a700 (LWP 25868)]
__memmove_avx_unaligned_erms ()
    at ../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:436
436	../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S: No such file or directory.
(...)
(gdb) backtrace
#0  __memmove_avx_unaligned_erms () at ../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:436
#1  0x00007fffeb739ae9 in gr::network::udp_source_impl::work(int, std::vector<void const*, std::allocator<void const*> >&, std::vector<void*, std::allocator<void*> >&) ()
    at /usr/lib/x86_64-linux-gnu/libgnuradio-network.so.3.10.5
#2  0x00007ffff6c6e87b in gr::sync_block::general_work(int, std::vector<int, std::allocator<int> >&, std::vector<void const*, std::allocator<void const*> >&, std::vector<void*, std::allocator<void*> >&)
    () at /usr/lib/x86_64-linux-gnu/libgnuradio-runtime.so.3.10.5
#3  0x00007ffff6c168dd in gr::block_executor::run_one_iteration() () at /usr/lib/x86_64-linux-gnu/libgnuradio-runtime.so.3.10.5
#4  0x00007ffff6c859a7 in gr::tpb_thread_body::tpb_thread_body(std::shared_ptr<gr::block>, std::shared_ptr<boost::barrier>, int) () at /usr/lib/x86_64-linux-gnu/libgnuradio-runtime.so.3.10.5
#5  0x00007ffff6c6cbdd in  () at /usr/lib/x86_64-linux-gnu/libgnuradio-runtime.so.3.10.5
#6  0x00007ffff6c6da47 in  () at /usr/lib/x86_64-linux-gnu/libgnuradio-runtime.so.3.10.5
#7  0x00007ffff661e43b in  () at /usr/lib/x86_64-linux-gnu/libboost_thread.so.1.71.0
#8  0x00007ffff7d9f609 in start_thread (arg=<optimized out>) at pthread_create.c:477
#9  0x00007ffff7ed9163 in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

[willcode]

@pamin98 I replicated the crash when a throttle is used. It shouldn't crash, but you do not want to use a throttle here because the UDP Source passes only data it receives, i.e., whatever is sending the UDP packets is acting as throttle.

@argilo Something in the new buffering scheme isn't handling overruns right.

[pamin98]

Even without a throttle block, the segmentation fault seems to persist though.
The gdb log for simple chain containing just a udp source and a file sink:
(...)
Thread 16 "udp_source1" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0xffffece4c1e0 (LWP 269391)]
__memcpy_generic () at ../sysdeps/aarch64/multiarch/../memcpy.S:182
182 ../sysdeps/aarch64/multiarch/../memcpy.S: No such file or directory.
(gdb) backtrace
#0 __memcpy_generic () at ../sysdeps/aarch64/multiarch/../memcpy.S:182
#1 0x0000ffffe71707d8 in gr::network::udp_source_impl::work(int, std::vector<void const*, std::allocator<void const*> >&, std::vector<void*, std::allocator<void*> >&) ()
from /lib/aarch64-linux-gnu/libgnuradio-network.so.3.10.5
#2 0x0000fffff6fbecac in gr::sync_block::general_work(int, std::vector<int, std::allocator >&, std::vector<void const*, std::allocator<void const*> >&, std::vector<void*, std::allocator<void*> >&)
() from /lib/aarch64-linux-gnu/libgnuradio-runtime.so.3.10.5
#3 0x0000fffff6f63074 in gr::block_executor::run_one_iteration() () from /lib/aarch64-linux-gnu/libgnuradio-runtime.so.3.10.5
#4 0x0000fffff6fd5ce8 in gr::tpb_thread_body::tpb_thread_body(std::shared_ptrgr::block, std::shared_ptrboost::barrier, int) () from /lib/aarch64-linux-gnu/libgnuradio-runtime.so.3.10.5
#5 0x0000fffff6fbce60 in ?? () from /lib/aarch64-linux-gnu/libgnuradio-runtime.so.3.10.5
#6 0x0000fffff6fbdc18 in ?? () from /lib/aarch64-linux-gnu/libgnuradio-runtime.so.3.10.5
#7 0x0000fffff6b7a624 in ?? () from /lib/aarch64-linux-gnu/libboost_thread.so.1.71.0
#8 0x0000fffff7e1051c in start_thread (arg=0xffffffffda8f) at pthread_create.c:477
#9 0x0000fffff7f0a22c in thread_start () at ../sysdeps/unix/sysv/linux/aarch64/clone.S:78

[willcode]

I think it's any time the buffer in the UDP Source fills. Is your packet source really fast? It could be that the file sink backs up due to disk speed, then the UDP Source backs up. It's still a bug. I'm just hoping there's something else you can do until it's fixed. Still, I'm glad you stress tested the block and found the bug!

[willcode]

I haven't been having much luck with my own advice above, so this block may be broken for the moment. Let's see what @argilo thinks.

[argilo]

It looks like the bug is with the overrun calculation here:

gnuradio/gr-network/lib/udp_source_impl.cc

Line 274 in 5d4076d

long overrun = bytes_read - d_localqueue_writer->bufsize();

d_localqueue_writer->bufsize() is an unsigned int, so bytes_read gets promoted to unsigned int as well, and then the conversion to long gives an enormous value close to 2^32.

[argilo]

Changing the type of bytes_read to long solves the problem.

[willcode]

Cool! I'll hold up 3.10.5.1 to get that fix in. Will that work on Windows?

[willcode]

asio::ip::udp::socket::receive_from() returns size_t, so that's probably the right answer.

[argilo]

It should work even if long is 32 bits, but I'll have to think about whether it would result in Undefined Behaviour.

I won't be able to make a pull request until tonight. Feel free to make your own if you'd like to get things moving sooner.

[willcode]

What do you think of this ... casting int from space_available() to size_t should be safe since it won't be negative.

diff --git a/gr-network/lib/udp_source_impl.cc b/gr-network/lib/udp_source_impl.cc
index 698f3bf01..1a28c12d3 100644
--- a/gr-network/lib/udp_source_impl.cc
+++ b/gr-network/lib/udp_source_impl.cc
@@ -251,7 +251,7 @@ int udp_source_impl::work(int noutput_items,
         }
     }
 
-    int bytes_read;
+    size_t bytes_read;
 
     // we could get here even if no data was received but there's still data in
     // the queue. however read blocks so we want to make sure we have data before
@@ -277,7 +277,7 @@ int udp_source_impl::work(int noutput_items,
                 read_data += overrun;
             }
 
-            if (d_localqueue_writer->space_available() < bytes_read)
+            if ((size_t)d_localqueue_writer->space_available() < bytes_read)
                 d_localqueue_reader->update_read_pointer(
                     bytes_read - d_localqueue_writer->space_available());
             memcpy(d_localqueue_writer->write_pointer(), read_data, bytes_read);

and it doesn't crash.

[willcode]

@argilo Thanks for tracking down the problem!

[pamin98]

Thank you for resolving the issue!