ncpamixer

ncpamixer copied to clipboard

Version: 1.3.3

Steps to reproduce:

1. Go to Configuration
2. Use ↑ to navigate beyond the top element

Humm, I can't reproduce this crash. It won't go past the top element it stops at the first one.

I thought it could've been due to my optimization flags (-Os, -s), but it happens even when I drop them.

One important note that this is built with clang (llvm 3.4.2), and traceback is this:

Program received signal SIGSEGV, Segmentation fault.
std::__1::__tree_prev<std::__1::__tree_node_base<void*>*> (__x=0x63d9d0 <pulse+112>) at /usr/include/c++/v1/__tree:172
172	    while (__tree_is_left_child(__x))
(gdb) bt
#0  std::__1::__tree_prev<std::__1::__tree_node_base<void*>*> (__x=0x63d9d0 <pulse+112>) at /usr/include/c++/v1/__tree:172
#1  0x000000000042926d in operator-- (this=0x7fffffffc5f0) at /usr/include/c++/v1/__tree:664
#2  operator-- (this=0x7fffffffc5f0) at /usr/include/c++/v1/map:690
#3  __advance<std::__1::__map_iterator<std::__1::__tree_iterator<std::__1::__value_type<unsigned int, PaObject*>, std::__1::__tree_node<std::__1::__value_type<unsigned int, PaObject*>, void*>*, long> > > (__i=..., __n=-1) at /usr/include/c++/v1/iterator:463
#4  advance<std::__1::__map_iterator<std::__1::__tree_iterator<std::__1::__value_type<unsigned int, PaObject*>, std::__1::__tree_node<std::__1::__value_type<unsigned int, PaObject*>, void*>*, long> > > (__i=..., __n=-1) at /usr/include/c++/v1/iterator:479
#5  prev<std::__1::__map_iterator<std::__1::__tree_iterator<std::__1::__value_type<unsigned int, PaObject*>, std::__1::__tree_node<std::__1::__value_type<unsigned int, PaObject*>, void*>*, long> > > (__x=..., __n=1) at /usr/include/c++/v1/iterator:527
#6  Tab::handleEvents (this=0x640ef0, event=0x7fffffffce31 "move_up") at /usr/src/ncpamixer-1.3.3/ui/tab.cpp:283
#7  0x0000000000423072 in Ui::handleInput (this=0x63dba0 <ui>) at /usr/src/ncpamixer-1.3.3/ui/ui.cpp:315
#8  0x000000000042331d in Ui::run (this=0x63dba0 <ui>) at /usr/src/ncpamixer-1.3.3/ui/ui.cpp:342
#9  0x000000000040b28b in main (argc=1, argv=0x7fffffffe428) at /usr/src/ncpamixer-1.3.3/pulsemixer.cpp:87

PulseAudio is 12.2, but I don't think it matters.

Maybe it has something to do with selected_index. I noticed another bug which may be linked to this one:

1. I go to Input Devices. It has 4 elements.
2. Navigating through them works fine so far, if you don't go beyond the top border.
3. When you hit ↑ while you're on the top element, you can't go below the second element with ↓ anymore. Unless you go to the top element and hit ↑ again. Now you can navigate down to the third element, but not the fourth. You repeat the process, and now you're able to navigate through all of them freely.

As soon as you press ↑ at the top element all cycle in 3. repeats all over again.

Ok, i think i know the problem. I will try to fix it.

Hey @c0r73x, did you have a chance to look at this?

I've done quite alot of rewrite in the dev branch. Can you test it and see if it fixes the problem?

Just tried dev branch. With a fresh build I get this on startup:

No audio engine specified!

Humm, it should use pulse as default. Did you run make distclean before make?

Ah, I see.

I'd put -DAUDIOENGINE="pulse" in CMakeLists.txt with cmake syntax and made it default there, rather than explicitly defining it in a custom Makefile. Since it's a very nasty mix of cmake and make.

I've run into a problem though due to an older version of cmake:

CMake Error at CMakeLists.txt:72 (add_compile_definitions): Unknown CMake command "add_compile_definitions".

It seems it's only available in 3.12, while I'm on the 3.9 branch.

In this case cmake_minimum_required(VERSION 3.1) should be replaced with cmake_minimum_required(VERSION 3.12) at the top of CMakeLists.txt to outline the minimum version with required features.

Ah, i will fix that :+1:

Temporarily I replaced it with add_definitions(-DUSE_PULSEAUDIO) for better portability.

And unfortunately the bug is still there.

Humm :( I will fix it, i'm planning to rewrite quite alot of the way the ui is drawn and will look into this at the same time.

From valgrind:

==9716== Invalid read of size 8
==9716==    at 0x431C5D: std::__1::__tree_node_base<void*>* std::__1::__tree_prev<std::__1::__tree_node_base<void*>*>(std::__1::__tree_node_base<void*>*) (__tree:172)
==9716==    by 0x42926C: operator-- (__tree:664)
==9716==    by 0x42926C: operator-- (map:690)
==9716==    by 0x42926C: __advance<std::__1::__map_iterator<std::__1::__tree_iterator<std::__1::__value_type<unsigned int, PaObject *>, std::__1::__tree_node<std::__1::__value_type<unsigned int, PaObject *>, void *> *, long> > > (iterator:463)
==9716==    by 0x42926C: advance<std::__1::__map_iterator<std::__1::__tree_iterator<std::__1::__value_type<unsigned int, PaObject *>, std::__1::__tree_node<std::__1::__value_type<unsigned int, PaObject *>, void *> *, long> > > (iterator:479)
==9716==    by 0x42926C: prev<std::__1::__map_iterator<std::__1::__tree_iterator<std::__1::__value_type<unsigned int, PaObject *>, std::__1::__tree_node<std::__1::__value_type<unsigned int, PaObject *>, void *> *, long> > > (iterator:527)
==9716==    by 0x42926C: Tab::handleEvents(char const*) (tab.cpp:283)
==9716==    by 0x423071: Ui::handleInput() (ui.cpp:315)
==9716==    by 0x42331C: Ui::run() (ui.cpp:342)
==9716==    by 0x40B28A: main (pulsemixer.cpp:87)
==9716==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
==9716==
==9716==
==9716== Process terminating with default action of signal 11 (SIGSEGV)
==9716==  Access not within mapped region at address 0x0
==9716==    at 0x431C5D: std::__1::__tree_node_base<void*>* std::__1::__tree_prev<std::__1::__tree_node_base<void*>*>(std::__1::__tree_node_base<void*>*) (__tree:172)
==9716==    by 0x42926C: operator-- (__tree:664)
==9716==    by 0x42926C: operator-- (map:690)
==9716==    by 0x42926C: __advance<std::__1::__map_iterator<std::__1::__tree_iterator<std::__1::__value_type<unsigned int, PaObject *>, std::__1::__tree_node<std::__1::__value_type<unsigned int, PaObject *>, void *> *, long> > > (iterator:463)
==9716==    by 0x42926C: advance<std::__1::__map_iterator<std::__1::__tree_iterator<std::__1::__value_type<unsigned int, PaObject *>, std::__1::__tree_node<std::__1::__value_type<unsigned int, PaObject *>, void *> *, long> > > (iterator:479)
==9716==    by 0x42926C: prev<std::__1::__map_iterator<std::__1::__tree_iterator<std::__1::__value_type<unsigned int, PaObject *>, std::__1::__tree_node<std::__1::__value_type<unsigned int, PaObject *>, void *> *, long> > > (iterator:527)
==9716==    by 0x42926C: Tab::handleEvents(char const*) (tab.cpp:283)
==9716==    by 0x423071: Ui::handleInput() (ui.cpp:315)
==9716==    by 0x42331C: Ui::run() (ui.cpp:342)
==9716==    by 0x40B28A: main (pulsemixer.cpp:87)

For now I applied this patch as a workaround, but it'd be nice to understand why it overflows on PA_CARDS:

--- ui/tab.cpp.orig     2022-08-10 01:41:43.000000000 -0700
+++ ui/tab.cpp  2022-08-23 21:56:41.555923735 -0700
@@ -275,6 +275,10 @@
             selected_block = object->size() - 1;
         }
     } else if (!strcmp("move_up", event)) {
+        if (selected_index == static_cast<uint32_t>(0)) {
+            return;
+        }
+
         auto i = std::prev(object->find(selected_index), 1);

         if (i != object->end()) {

I cannot reproduce this issue anymore.

Not sure what commit was supposed to fix it. I built 1.3.3.5 and the bug was still there. Can't build anything after that because the project went straight to C++17. My old box is too old for that -- with only gcc 4.6 / clang 3.4 available.

@c0r73x I can revert the C++17, it's your call. I wish there was a workaround, like linking everything statically, and releasing binaries. But that would probably take some work. It's prob easier if I just implement the std::filesystem functions used from C++17 and so that users can still build on older systems.

Regarding the issue itself, I'll take the time to look into this with more details. I guess this patch fixes the issue but there are still questions?

@tricktux yeah the main question was why it wasn't reproducible by others.

Also the problem described in https://github.com/fulhax/ncpamixer/issues/26#issuecomment-482984691 was also reproducible in 1.3.3.5.

I cannot reproduce the issue mentioned in the comment. I guess has to do with an older version of pulseaudio returning something weird. I'll look closer at that code path later.

@c0r73x I can revert the C++17, it's your call. I wish there was a workaround, like linking everything statically, and releasing binaries. But that would probably take some work. It's prob easier if I just implement the std::filesystem functions used from C++17 and so that users can still build on older systems.

Regarding the issue itself, I'll take the time to look into this with more details. I guess this patch fixes the issue but there are still questions?

Sure, go ahead