Various minor issue from scan-build output

[acmay]

Running scan-build on the make shows some possible problems. I fixed a few of the minor ones but there are some others where I don't know the code well enough to attempt to fix them yet.

I don't have a clone setup to do a straight PR but here are some patches for them. patches.zip

Here are the toplevel info on some of them. I can post the html results if you aren't able to run scan-build.

Logic error Dereference of null pointer player.c source_pause 1050 12 View Report Logic error Dereference of null pointer httpd_streaming.c streaming_fail_cb 102 5 View Report Logic error Dereference of null pointer httpd_streaming.c streaming_fail_cb 104 7 View Report Logic error Dereference of null pointer RSP2SQL.c expr 1181 27 View Report Logic error Dereference of null pointer RSP2SQL.c expr 1276 27 View Report

[ejurgensen]

Thanks again. I wasn't aware of scan-build, but it seems very useful. Went through the issues and think I have them fixed now (commit bdd6bab9824be4b440f43ddea4749caf2b284a9d). I didn't apply the patches, because in a number of the cases I decided to take a slightly different approach.

@chme, there is still a memleak in queue.c (very much an edge case, though), which I hope you can fix. I think there is list that may need to be free'd. There are also a few dead assignments in mpd.c. You can find them with scan-build.

[acmay]

No problem. Thanks for fixing the things up. I was hoping scan-build would have caught that other crash I ran into but it didn't. If you want you can sign up to use coverity for open source projects and it can do a better job at finding things. https://scan.coverity.com/

I always like to take advantage of the fact that it is safe to free(NULL), so you can stick with the if (!os || !ps) rather than breaking it into 2 if blocks. I have seen too many cut-paste typos on that type of stuff.

[ejurgensen]

Yes, you are right about the cut-n-paste risk this gives.

There is a lot of out-of-memory handling in forked-daapd (which ironically probably uses a quite some kilobytes on different log messages), and I'm not sure it really makes sense. I'm tempted to either scrap it all (there are already many strdup's without any checking), or make some sort of common wrapper that will write a nice error message to the log and then terminate forked-daapd.

Do you or @chme have any thoughts on that?

[chme]

I find this a good analysis of the out-of-memory handling: http://eli.thegreenplace.net/2009/10/30/handling-out-of-memory-conditions-in-c

Conclusion for applications is to use a wrapper that aborts on oom.

[ejurgensen]

Thanks, very interesting read. A wrapper it is then! Maybe it should be called with FILE and LINE to log that on failure? On the other hand OOM wouldn't necessarily be related to the specific allocation.

[acmay]

One problem with wrappers is that it can make it harder for things like dmalloc or valgrind from helping you track down leaks. If you can't take the overhead of doing a full call chain recording, you just get the single return address recorded with the alloc, and then every return address would just point back to your alloc wrapper. Valgrind may be better, but I typically work on embedded targets where you don't have that option. http://dmalloc.com/docs/latest/online/dmalloc_10.html#SEC12

Since daapd can be playing untrusted files off a network you should look at the possible security issues as well. If a file has a bad value that causes a malloc( -1 ); you would just want an error rather than a abort/crash.

Then there are a few things that do need to be wrapped. malloc/calloc/realloc/strdup/strndup

If you do want to do a wrapper I would suggest a macro to at least help revert back to the raw calls if needed. And it is an easier search/replace than putting the file/line on each one.

#ifdef WRAP_ALLOCS
#define my_alloc(size_t size) my_wrapped_alloc(size, __FILE__,__LINE__)
#else
#define my_alloc(size_t size) malloc(size)
#endif

I am not sure if this is a common issue but I have seen problems with binary reproducible builds with FILE. Usually it is ok, but if it is in a header file, then you can get the full path in the binary which can be bad. Here is some more info on the topic. https://wiki.debian.org/ReproducibleBuilds/About

[ejurgensen]

@acmay, thanks and very interesting as well. I see the point of the macro.

Maybe FILE isn't strictly necessary, perhaps the log domain is enough with a line number. You don't seem to hooked on wrappers, but I am unsure what your preferences then are?

You point to an issue that is relevant, namely that there many variations of malloc. In addition to those you mention, there are also all the library specific ones, like libevent_new and so on. Wrapping them all seems a bit much, and maybe instead of wrapping a subset, it would be better to just leave them all and instead exchange all the following "if(!mem)"-checking with something kind of like assert?

That would of course not support sanity checking of allocation size requests.