On 19 February 2015 at 05:05, Peter Kelly <[email protected]> wrote:
> > On 19 Feb 2015, at 4:19 am, Gabriela Gibson <[email protected]> > wrote: > > > > Hi, > > > > Reading through the source, I see that a lot of mallocs() do not have a > > > > [[ > > if(foo == NULL) { > > perror("Foo: Out of memory.\n"); > > return _exit(EXIT_FAILURE); > > } > > ]] > > Strictly speaking, one is supposed to check the result of every malloc > call, and handle that in a graceful manner - as you’ve suggested. This, > however, comes with costs - and those are very significant costs - in terms > of having a lot of extra code to do this checking, for what I would argue > is minimal benefit. > I do agree that we should avoid exploding the code, however there are elegant solutions to that. I am a strong believer in "believe is good, control is better", meaning don´t trust such functions. Corinthia also works on small devices, and just think of someone opening a big complicated document on a smartphone...that is nearly bound to cause memory problems. Having the checks in code, makes us search for the problems in the right place....e.g. returning a NULL pointer might cause a crash in a very different place. > > I just did a quick grep through the sources for the main functions that > call the C library’s memory allocator, and came up with these counts: > > malloc 37 > calloc 73 > realloc 8 > strdup 194 > > So this is a total of 312 places in the code you’d need to add the above. > > Most C code I’ve seen either ignores failures of malloc and similar > functions, or alternatively wraps them in functions which do the exit check > themselves, typically called xmalloc, xcalloc, xrealloc, xstrdup, etc. > Reading Dennis’s email, this is the same as what he’s suggested. So we > could do this, and cause a process exit whenever memory allocation fails. > I have been toying with that idea, so I would welcome 2 functions in platform e.g. DFplatformMalloc() and DFplatformFree(). Having those functions, means a couple of things: - The caller does not need to check (code savings) - Memory leakage (in our code) detection becomes a factor easier - We can, if we want to, build our own memory manager and save a lot of costly malloc() calls (especially expensive in Windows and Android). - the overhead of a function call is not a concern. Caller -> DFplatformMalloc() -> malloc() - We can, in once place, control the preferred error handling. > > My question is whether this is really the best way to handle the problem, > or if it needs to be handled at all (in a library). DocFormats itself is > not something that runs standalone; it’s always embedded in an application > (dftest and dfconvert are two examples, but the intention is there’d be > larger examples). So I think that if we’re going to add error handling, it > would be much better to delegate that to the application itself, rather > than taking the decision to terminate the process ourselves. > I agree with your pow, but this is a bigger theme than just malloc() I would like the application that uses DocFormats to in a setup call, give a function pointer....and whenever we have a crash/exit condition we call the function pointer, with a text and maybe also __FILE__ and __LINENO__ That would give us the ultimate flexibility. Gabriela@ watch out, this could be your next project. > > Currently if DocFormats experiences a memory allocation failure, it > crashes, and the process exits. With a malloc (and similar functions) check > that calls exit, then wehn a memory allocation fails, the process exits. > Both are equally undesirable situations from an application’s point of > view, so I don’t think we buy anything from either the explicit exit call > like above, or wrapper functions that do this. > I agree with you....but how about we let Gabriela centralize malloc() free() first...and then do e.g. as I suggested above with a function pointer. > > On a desktop OS, an application could launch a sub-process, either via > fork() on Unix or CreateProcess on Windows, so that a fatal error leading > to a crash or exit call during the conversion would not affect the > application itself. Unfortunately some platforms (I’m looking at you, iOS) > don’t allow fork(), and everything has to be done within the one process. > I agree.....android is even worse, it allows fork(), but the processes remain dependent of each other, and even share some resources like IO pool. > > About a year ago I ported TeX to iOS, and it had calls all over the place > that would simply exit if it had a problem, under the (usually correct) > assumption that it’d be running as a sub-process, and the caller could just > continue on normally. But if you do this in an iOS app, then the user is > unceremoniously dumped back to the home screen, without any indication as > to what has gone wrong. > Exit in the normal code, should not be allowed......we could in these places call a central function, that could then determine what to do. Not using exit() is NOT an excuse not to carry out tests and make the code more robust! > In practice, memory allocation failures are extremely unlikely, but if we > do want to handle them gracefully then I argue we should do it properly - > calling exit() gives you practically no benefit over simply crashing. A way > to handle this that avoids either case is to use exception handling - > setjmp() and longjmp() in C. This is how I handled the case of TeX’s exit() > calls; I would simply call setjmp() before the start of the TeX run, and > then replace the calls to exit() with longjmp(). > This is an elegant way...I would however leave that to the application that uses DocFormats (see idea of function pointer) > > The next thing to address i cleanup. If a malloc failure occurs deep in a > conversion run, and we jump out to an exception handler, we should free all > memory that was allocated during that run. This means wrapping > malloc/calloc/realloc/strdup and any other functions in the C library, and > using our own data structure to keep track of what has been allocated (and > also to update this data structure when memory is freed). Then, if an error > occurs, after we return to the call site of setjmp() and go into the > exception handling block, we go through this data structure and call free() > on all the blocks of memory that were allocated by DocFormats, so the > application can then show an error message and then continue operating. > Now this is quite a different discussion.....do we want to recover gracefully from severe errors or simply make sure the whole application terminates. I advocate that we cannot recover gracefully from all severe errors, so no reason breaking our backs trying. > > TL;DR - calling exit() doesn’t give any practical benefit over simply > crashing; if we want to handle out-of-memory errors we need should do so > cleanly in a way that allows the application using the library to continue > running, and that means tracking & subsequently freeing all memory > allocated by DocFormats, to avoid memory leaks and further out-of-memory > errors. > I do not agree to this.....we will encounter severe errors (no memory being one) that cannot be handled gracefully, in that situation it is better to restart at application level. That being said, I also believe calling exit() in a library is BAD, because it does not give the application a chance to do anything. I suggest that Gabriella takes the input from us, and writes what she propose to do. Rgds jan I. > > — > Dr Peter M. Kelly > [email protected] > Wonders with a big smile, if I should write "double V.P." :-) > > PGP key: http://www.kellypmk.net/pgp-key <http://www.kellypmk.net/pgp-key> > (fingerprint 5435 6718 59F0 DD1F BFA0 5E46 2523 BAA1 44AE 2966) > >
