radarsat1 / liblo

liblo is an implementation of the Open Sound Control protocol for POSIX systems
GNU Lesser General Public License v2.1
192 stars 60 forks source link

Segmentation fault when starting two servers with same port in C++ #81

Closed theGreatWhiteShark closed 4 years ago

theGreatWhiteShark commented 5 years ago

This seems related to #41 but still occurs on current master.

Starting two ServerThreads using the same port causes a Segmentation fault.

#include <lo/lo.h>
#include <lo/lo_cpp.h>

int main(){
    lo::ServerThread st(9000);
    lo::ServerThread st2(9000);
    st.start();
    st2.start();
}

Since I don't see a way to query for the port numbers already in use via liblo (of course not), this becomes quite troublesome when allowing the user to both create multiple instances of an application and, at the same time, to customize the OSC port used.

radarsat1 commented 5 years ago

Thanks! In general the C++ wrapper doesn't do a validity check before operations on the underlying lo_server/lo_server_thread. However, maybe this would make sense for start() but then it would also have to return a boolean.

Is there any reason is_valid() is not sufficient for your application?

I would say that here the segfault saved you from proceeding with an invalid server. Maybe a debug-mode assertion would be more appropriate than a segfault at the very least.

theGreatWhiteShark commented 5 years ago

Yes, is_valid() is totally sufficient for my application. I simply didn't thought it would check for the availability of the specified port since I didn't found its counterpart in the C API documentation or its description in the header files. Thanks a lot!

Indeed, having a invalid server would have been way harder to figure out. But for me the is_valid() solved the issue.

radarsat1 commented 5 years ago

Great! Yes, the reason there is no counterpart is that lo_server_new simply returns NULL in that case. However, in C++ there is no way to return an error from a constructor. The only alternative would have been to raise an exception but I didn't want to put exceptions in the code, so I compromised with is_valid. I will try to add some assertions to help people detect mistakes with invalid servers so that they don't have to reverse engineer a segfault. I'll leave this open until that gets done.

radarsat1 commented 4 years ago

I took a shot to address this more completely in 19f60b82d54a4f87626910dae49e9ecbdd29dd09

theGreatWhiteShark commented 4 years ago

Nice. Thanks a lot!

radarsat1 commented 4 years ago

Great, yes I ended up adding optional exceptions, which defaults to assertions if not desired, I think that should be the best of both worlds. I'll add tests for it and then close this issue if all seems good.

theGreatWhiteShark commented 4 years ago

:+1:

radarsat1 commented 4 years ago

Closed in 8e45b2409db2a736b542b85ea9c8dd0925a2cefd