GNOME Bugzilla – Bug 742019
Setting musicbrainz proxy values to NULL causing segfault
Last modified: 2015-01-23 13:09:02 UTC
Created attachment 293374 [details] [review] Potential fix Trying to run sound-juicer on FreeBSD 11.0-CURRENT results in a segfault: (gdb) bt
+ Trace 234459
Using "" instead of NULL for the values passed to MB seems to fix the issue. (std:string not liking NULL?) Patch attached. My C++ fu is non-existant, so I'm not sure how right the fix is. Thanks.
Thanks for taking the time to report this problem. I've just checked and on linux there is no problem passing NULL to mb5_query_set_proxyhost(). I think it would be worth digging a bit deeper to find out what the problem is on FreeBSD, it could be libmusicbraniz, neon or libstdc++. Are you able to get a back trace with debuginfo so we can see where in mb5_query_set_proxyhost() happens please?
I think this is blowing up deep in libstdc++, and I haven't found an easy way yet to get debuginfo for that. AFAICT, the following is happening: b5_query_set_proxyhost(Mb5Query Query, const char *ProxyHost) just calls MusicBrainz5::CQuery::SetProxyHost(const std::string& ProxyHost) (Generated by the macros found in src/md5_c.cc in the libmusicbrainz source) Which invokes the std::string constructor automatically. But everything I've found digging into std:string says that passing in a NULL pointer to std::string constructor results in undefined behaviour. This may seem to work on Linux, but I think because the behaviour is undefined, it is not guaranteed to and is resulting in problems on FreeBSD 11. Ultimately, to avoid the undefined behaviour, I think that at some point that "" needs to be passed to the std::string constructor and not NULL. I don't know if libmusicbrainz should be doing this for us, or if SJ should be passing it. Thoughts? Am I understanding this right? References: * http://stackoverflow.com/questions/17464514/initialize-stdstring-from-a-possibly-null-char-pointer * http://www.cplusplus.com/reference/string/string/string/
Thanks for looking into the problem, I think your assessment is correct, it sounds like libstdc++ on FreeBSD is less forgiving than on linux but is complying with the c++ standard. I think it is worth raising this with the musicbraniz developers to see if they will fix it there so that it gets fixed for everyone. Are you happy to raise it with them? If not I can do it next week.
I've tested a fix in libmusicbrainz that makes this work fine w/ the current SJ code, and submitted a PR for their project on GitHub: https://github.com/metabrainz/libmusicbrainz/pull/6 We'll see what comes of that discussion.
Comment on the GitHub PR from a libmusicbrainz dev: "To be honest, I think it's quite clear that the C interface is expecting a null terminated string here, so ultimately the calling application should be responsible for passing correct data. I'd suggest an update to sound-juicer to ensure that it passes an empty string (or doesn't call the method at all) if no proxy is required."
Comment on attachment 293374 [details] [review] Potential fix Thanks for following this up with the libmusicbrainz developers. As they are reluctant to fix it there I've applied your patch with some changes to the comments. Attachment 293374 [details] pushed as 4ee58ce - Fix crash on FreeBSD