[Development] QtCore missing check for memory allocation

Oswald Buddenhagen oswald.buddenhagen at theqtcompany.com
Fri Feb 27 09:20:54 CET 2015


On Thu, Feb 26, 2015 at 04:12:27PM -0800, Thiago Macieira wrote:
> On Thursday 26 February 2015 20:54:32 Oswald Buddenhagen wrote:
> > > Let me put it this way: who's going to write the unit tests to ensure we
> > > get coverage for all those conditionals? Any volunteers?
> > 
> > which conditionals? the malloc wrapper would throw/qFatal (depending on
> > the build configuration). your dream of never-failing malloc would be a
> > reality.
> > or we "just" use new() everywhere. this one already has said wrapper.
> 
> The conditionals for checking the result of malloc or new (nothrow).
>
the whole point would be *not* using unwrapped malloc and new(nothrow).
this can be trivially verified for our own code with a grepping bot.

> > regarding replacing the malloc implementation ... is it possible to link
> > an implementation that would actually throw?
> 
> No, malloc() cannot ever throw. If you throw and your caller is C code, you 
> crash. Also, malloc() is defined as nothrow() in the glibc headers.
> 
yeah, makes sense.

> > > Sorry, the compiler *is* allowed to remove undefined behaviour because, by
> > > the very definition of undefined behaviour, ANYTHING can happen,
> > > including absolutely nothing.
> > 
> > then explain edd2d9bd0a7f5dbe059aea0902d519b728acc01a.
> 
> Who says that's good code? I fixed a warning, I didn't make the code good.
> 
> If the compiler starts removing the undefined behaviour, we'll get a test 
> failure and we can easily correct it. We can't do that in a library because we 
> don't know when it will fail.
> 
clang suggests to do precisely what the patch did. are you saying it is
making an unsafe suggestion?

> The argument is that it implies runtime overhead. See Robin's email for 
> numbers. This is asking for making the code slower on the very devices where 
> it needs to run faster.
> 
i don't trust this number. i don't know how qMalloc was implemented, but
there is no way a simple forwarding wrapper would add 10% overhead to
malloc (esp. in an optimized build).
modern processors even have a specific optimization for call forwarding
(or whatever it's called properly).

> The argument is that we're in the habit of not checking it, so even if we had 
> a massive effort to fix all the cases, we'll soon add more that we don't check. 
> 
> The argument is that we have no infrastructure for testing OOM situations to 
> guarantee that we reliably take the action we decided. And no one is 
> volunteering to write the tests. Just sprinkling Q_CHECK_PTR all over the 
> place is no good if we miss critical ones.
> 
> The argument is that this is a massive change. Most of our modules (except for 
> QtCore, QtConcurrent, QtXmlPatterns) are compiled with exceptions disabled and 
> yet we use the regular, non-nothrow operator new, so we'd need to replace it 
> EVERYWHERE with the nothrow version plus the check.
> 
why are you basing your argumentation on the opposite of what we are
suggesting? reliable failure is what ulf asked for. throw/qFatal everywhere.

> The argument is that we use libraries that don't care either for OOM 
> situations. So even if we made the effort ourselves, it might be all for 
> nothing as soon as we make a call to one of those libraries (think glib & 
> friends).
> 
this is a good argument.
luckily, user code that cares can avoid this pitfall to a high degree by
either avoiding the functionality at all (compiling without glib, for
example), or using improved "backend" libraries.

> The argument is that even if we happened to catch all failures to malloc() in 
> all libraries, there's also all those pesky system calls that may return 
> ENOMEM and we're not checking those. So wrapping malloc() and operator new are 
> not enough: we need to check system calls too. And extend that to all the 
> calls to libraries that do catch malloc() and system call failures and report 
> errors to us.
> 
the failure to check the return value of system calls needs to be
addressed irrespective of what we decide about memory management. it's
simply irresponsible to not handle syscall return values, and can even
lead to security holes.

> The argument is that chain is as strong as its weakest link. If we don't fix 
> all the holes, the bucket will still leak (pardon my switch of metaphors).
> 
you can let this be somebody else's worry. our task is to enable a use
case, not to make it bullet-proof ourselves. the first step is not being
actively hostile to it.

> The argument is that, even with all of this, there's no way to reliably catch 
> an OOM situation, due to overcommit. The application may suddenly disappear.
> 
not on sane(ly configured) embedded systems.

> And even if I had no technical arguments, my opinion counts. I'm not pulling 
> this out of thin air. I have over a decade of experience, so if I say that it 
> sounds wrong, it probably is. And I'm the maintainer for QtCore, so I'm the 
> one that needs to be convinced, not the other way around.
> 
you can pull an argument to authority when no unanswered technical
arguments remain and it's still a draw.



More information about the Development mailing list