Hi,> On Fri, 2011-04-29 at 09:05 +0200, Peter J. Philipp wrote: >> You don't check for malloc failure. I've made a patch that is possibly >> wrong but it saves the program from SIGSEGV and replaces it with SIGABRT.On Fri, 29 Apr 2011, Philipp Schafft wrote:> But I have a question: > Not all of them (only had a brief look at the patch) look to be in a > 'safe' startup/shutdown state but code wich is run within the normal > operation. Wouldn't it be better to handle those cases in a diffrent way > because abort() will kill the process? Currently if it derefences NULL > the kernel will kill the process (very likely). This patch would improve > the situation because the behavor will be more defined wich is a big pro > already.Most implementations of malloc() don't return NULL except for very obvious cases of bad programming. When memory is running out, malloc() may return a valid pointer, but writing anything in the malloc-ed data region can still result in the kernel killing the process. So, although I think checking the return values is a good thing, this is no guarantee at all that things will always run as expected. (Besides, when running icecast on a Linux box, there's always the risk of being killed by the OOM killer even when you're not doing anything wrong.) Just my 2 cents.. -- Maarten
2011/5/7 Maarten Bezemer <mcbicecast at robuust.nl>:> Hi, > >> On Fri, 2011-04-29 at 09:05 +0200, Peter J. Philipp wrote: >>> You don't check for malloc failure. ?I've made a patch that is possibly >>> wrong but it saves the program from SIGSEGV and replaces it with SIGABRT. > > On Fri, 29 Apr 2011, Philipp Schafft wrote: >> But I have a question: >> Not all of them (only had a brief look at the patch) look to be in a >> 'safe' startup/shutdown state but code wich is run within the normal >> operation. Wouldn't it be better to handle those cases in a diffrent way >> because abort() will kill the process? Currently if it derefences NULL >> the kernel will kill the process (very likely). This patch would improve >> the situation because the behavor will be more defined wich is a big pro >> already. > > Most implementations of malloc() don't return NULL except for very obvious > cases of bad programming. > When memory is running out, malloc() may return a valid pointer, but > writing anything in the malloc-ed data region can still result in the > kernel killing the process.Hmm.. So what am I not reading right in this: "If successful, calloc(), malloc(), realloc(), reallocf(), and valloc() functions return a pointer to allocated memory. If there is an error, they return a NULL pointer and set errno" Running out of memory is not considered as an error when calling malloc? Romain
On Sat, 7 May 2011, Romain Beauxis wrote:> Hmm.. So what am I not reading right in this: > "If successful, calloc(), malloc(), realloc(), reallocf(), and > valloc() functions return a pointer to allocated memory. If there is > an error, they return a NULL pointer and set errno" > > Running out of memory is not considered as an error when calling malloc?
On 05/08/2011 01:06 AM, Romain Beauxis wrote:> Running out of memory is not considered as an error when calling malloc?On linux, the only way to get an error when calling malloc() is to disable memory-overcommiting. On regular linux systems, this is the _only_ way for malloc() to return NULL. If an icecast process reaches that point, it's screwed anyway it won't be able to do anything meaningful: any source reads will fail (refbuf alloc), any log print will also fail (printf uses malloc too), so it might as well give up and call abort(). However, even doing that is useless for 2 reasons: - adding abort()s everywhere bloats the code and add code paths that are unlikely to be tested. Complex test suites are mandatory for this to succeed, otherwise icecast may not work as intended when encountering a malloc() error. - as has been said by Maarten, memory errors are much more likely (I'm talking orders of magnitude) to manifest themselves as SIGKILL rather than malloc() returning NULL. Bottom line, checking what malloc() returns is just useless on modern desktop/server operating systems, don't do it. Instead, icecast should keep its memory footprint low and avoid leaks at all costs. Even with massive numbers of sources/clients, these 2 goals alone should keep icecast under the OOM Killer's radar. Here are a couple references: http://blog.ometer.com/2008/02/04/out-of-memory-handling-d-bus-experience/ http://article.gmane.org/gmane.comp.audio.jackit/19998 Cheers, R?mi