runtime: Use MAP_FIXED flag to ensure buffer halves are contiguous

[argilo]

Description

It was recently reported that Gqrx crashes on FreeBSD:

gqrx-sdr/gqrx#1275

After some digging, I found that the crash occurs because vmcircbuf_mmap_shm_open::vmcircbuf_mmap_shm_open allocates a discontiguous double-mapped buffer. This happens because the mmap system call is free to ignore the supplied addr argument (which is only a suggestion) and choose its own address for the mapping.

This can be fixed by using the MAP_FIXED flag, which tells mmap that the addr argument must be used as-is.

The vmcircbuf_mmap_tmpfile implementation has an explicit contiguity check (which simply throws an exception if the returned buffer is discontiguous). With the addition of the MAP_FIXED flag, I don't think this check is necessary.

Related Issue

• SIGSEGV with memcpy() in rx_meter_c::work() (src/dsp/rx_meter.cpp:73) on FreeBSD gqrx-sdr/gqrx#1275

Which blocks/areas does this affect?

Circular Buffers

Testing Done

To test the mmap-based circular buffer implementations, run the following:

echo -n gr::vmcircbuf_mmap_shm_open_factory > ~/.gnuradio/prefs/vmcircbuf_default_factory
OR
echo -n gr::vmcircbuf_mmap_tmpfile_factory > ~/.gnuradio/prefs/vmcircbuf_default_factory

Before this change, gr::vmcircbuf_mmap_shm_open_factory works fine on Linux, but gr::vmcircbuf_mmap_tmpfile_factory fails immediately with the following error:

gr::vmcircbuf :error: non-contiguous second copy
buffer_double_mapped :error: gr::buffer::allocate_buffer: failed to allocate buffer of size 65536 KB

After the change, Gqrx runs fine with both implementations.

Checklist

• I have read the CONTRIBUTING document.
• I have squashed my commits to have one significant change per commit.
• I have signed my commits before making this PR
• My code follows the code style of this project. See GREP1.md.
• I have updated the documentation where necessary.
• I have added tests to cover my changes, and all previous tests pass.

[willcode]

Looking at the man page, I wonder if MAP_FIXED_NOREPLACE is more appropriate, though it was introduced in 4.17. The bit about MAP_FIXED that says

If the memory region specified by addr and length  overlaps  pages of  any existing mapping(s),
then the overlapped part of the existing mapping(s) will be discarded.

Is a little worrying in the case of a race.

But ... there is some text later on in the man page that says MAP_FIXED is safe if the range was previously reserved using another mapping. It could be that the checks in the code are sufficient.

[argilo]

MAP_FIXED_NOREPLACE appears to be Linux-specific, so it might not be much use if these implementations aren't typically used on Linux.

[willcode]

@marcusmueller @mormj This fix seem simple enough, but the code predates 3.7, isn't tested by CI, so another opinion would be good.

[argilo]

Out of curiosity, I had a look back and the original pspectra / GNU Radio 0.9 code used the MAP_FIXED flag, but by the time circular buffers were re-implemented in GNU Radio 2.2 it was no longer there.

[argilo]

there is some text later on in the man page that says MAP_FIXED is safe if the range was previously reserved using another mapping.

From https://pubs.opengroup.org/onlinepubs/9699919799/functions/mmap.html:

If a MAP_FIXED request is successful, then any previous mappings or memory locks for those whole pages containing any part of the address range [pa,pa+len) shall be removed, as if by an appropriate call to munmap(), before the new mapping is established.

This makes me wonder whether the explicit munmap calls should be removed, since their presence could result in a race condition.

[argilo]

The spec goes on to say some worrying things about MAP_FIXED:

Use of MAP_FIXED may result in unspecified behavior in further use of malloc() and shmat(). The use of MAP_FIXED is discouraged, as it may prevent an implementation from making the most effective use of resources. Most implementations require that off and addr are multiples of the page size as returned by sysconf().

[...]

The MAP_FIXED address treatment is likely to fail for non-page-aligned values and for certain architecture-dependent address ranges. Conforming implementations cannot count on being able to choose address values for MAP_FIXED without utilizing non-portable, implementation-defined knowledge. Nonetheless, MAP_FIXED is provided as a standard interface conforming to existing practice for utilizing such knowledge when it is available.

Similarly, in order to allow implementations that do not support virtual addresses, support for directly specifying any mapping addresses via MAP_FIXED is not required and thus a conforming application may not count on it.

[argilo]

GNU Radio's test suite can be executed using the three non-Windows circular buffer implementations like so:

echo -n gr::vmcircbuf_sysv_shm_factory > ~/.gnuradio/prefs/vmcircbuf_default_factory
ctest -j8
echo -n gr::vmcircbuf_mmap_shm_open_factory > ~/.gnuradio/prefs/vmcircbuf_default_factory
ctest -j8
echo -n gr::vmcircbuf_mmap_tmpfile_factory > ~/.gnuradio/prefs/vmcircbuf_default_factory
ctest -j8

All tests pass on this branch, and they continue to do so if I remove the munmap calls that precede the allocation of second_copy.

[argilo]

I've removed the munmap calls as well.

[marcusmueller]

yep, MAP_FIXED is the right thing to do here. Still wondering why it wasn't there in the first place. Not quite sure we even need the MAP_SHARED, but that's a story for a different day.

[marcusmueller]

Out of curiosity, I had a look back and the original pspectra / GNU Radio 0.9 code used the MAP_FIXED flag, but by the time circular buffers were re-implemented in GNU Radio 2.2 it was no longer there.

that's some serious history digging there :)

[willcode]

@argilo When you're ready, I'll put this on main. It doesn't seem like most users will ever see the change on Linux/Windows. Don't know about MacOS (sounds like it supports sysv but shm open is recommended?). So it may be only FreeBSD that even notices shm open, and tmp might not be used at all.

[argilo]

tmp might not be used at all

It was removed in 2005, but that broke NetBSD:

https://lists.gnu.org/archive/html/discuss-gnuradio/2005-10/msg00074.html

After some discussion on the mailing list, it was eventually added back:

https://lists.gnu.org/archive/html/discuss-gnuradio/2005-10/msg00109.html

Whether NetBSD still requires vmcircbuf_mmap_tmpfile is another question.

[argilo]

When you're ready, I'll put this on main.

In gqrx-sdr/gqrx#1275 (comment), I asked @martymac to take this for a spin on FreeBSD, so we may as well wait a day or two to see how that goes.

[argilo]

I took this for a spin myself on FreeBSD (by applying the patch to the GNU Radio 3.8.4.0 port), and both stand-alone GNU Radio flow graphs and Gqrx now run without issue, even with ASLR enabled.

[willcode]

Good, then let's merge this to main and see if anyone has issues.

I've noticed that the sysv method leaves the guard pages (->) mapped. These two listings are from /proc/PID/maps and ipcs. I don't know if this wastes resources. The FG here is null source, throttle, null sink, so there are two output buffers.

-> 7fd8b850a000-7fd8b850b000 r--s 00000000 00:01 1572878                    /SYSV00000000 (deleted)
   7fd8b850b000-7fd8b8d0b000 rw-s 00000000 00:01 1572880                    /SYSV00000000 (deleted)
   7fd8b8d0b000-7fd8b950b000 rw-s 00000000 00:01 1572880                    /SYSV00000000 (deleted)
-> 7fd8b950b000-7fd8b950c000 r--s 00000000 00:01 1572878                    /SYSV00000000 (deleted)
-> 7fd8b981e000-7fd8b981f000 r--s 00000000 00:01 1572881                    /SYSV00000000 (deleted)
   7fd8b981f000-7fd8b982f000 rw-s 00000000 00:01 1572883                    /SYSV00000000 (deleted)
   7fd8b982f000-7fd8b983f000 rw-s 00000000 00:01 1572883                    /SYSV00000000 (deleted)
-> 7fd8b983f000-7fd8b9840000 r--s 00000000 00:01 1572881                    /SYSV00000000 (deleted)

-> 0x00000000 1572878    ...        400        4096       2          dest         
   0x00000000 1572880    ...        700        8388608    2          dest         
-> 0x00000000 1572881    ...        400        4096       2          dest         
   0x00000000 1572883    ...        700        65536      2          dest         

[martymac]

Thanks a lot for your investigations, Clayton and Jeff!

Sorry for lagging (ENOTIME at the moment). I'll test the patch and tell you how it goes (but it seems it now goes well on FreeBSD :)).

[argilo]

Whether NetBSD still requires vmcircbuf_mmap_tmpfile is another question.

It appears NetBSD has had shm_open since 7.0 (ca. 2013): https://man.netbsd.org/NetBSD-7.0/shm_open.3

[willcode]

Then you're probably the first person to test the tmp method in years. And it still worked ... they don't make code like they used to 😄

[argilo]

And it still worked

Well, it did better than the shm_open version because it detected the discontiguous buffer, but I'm not sure I'd count it as working when all it did was throw an exception 😄.

[martymac]

Hello,

I can confirm that Gqrx works perfectly well (even with ASLR) with your patch! Thanks again for your help!