Re: Checking string allocations (was Re: String literals, ASCII vs UTF-8)

On Wednesday 29 of February 2012, Caolán McNamara wrote:
> On Wed, 2012-02-29 at 17:11 +0100, Lubos Lunak wrote:
> > Do we actually have code that tries to gracefully handle running out of
> > memory? Because if not, and I doubt we realistically do[*]
>
> Its not O[U]String related, but FWIW vcl/unx/source/gdi/salbmp.cxx has
> some std::bad_alloc catches from commit 807d9a7d for
> https://issues.apache.org/ooo/show_bug.cgi?id=82997 which has a
> test .png of https://issues.apache.org/ooo/attachment.cgi?id=49179 as an
> apparent real-world case.

 I don't know how this works in practice, but if only X11SalBitmap itself 
handles the situation well and code using the class does not (and in the case 
of an import filter just silently drops the code), then I still do not 
consider that graceful handling.

> dunno about the idea of making rtl_allocateMemory simply abort, I mean a
> lot of, especially the .doc/.ppt/.xls, filters are still based on a
> certain degree of guesswork as they were written before specs got
> released, we can sometimes end up parsing a pile of random junk in the
> delusion its something else, even outside all the hacked documents which
> deliberately attempt DoS. Just crashing and taking out every open
> document that user's got when opening a random .ppt vs showing as much
> as we can, or at least just abandoning the load attempt, is a hard
> sell.

 Yes, it is a choice between possibly silently dropping parts of documents and 
possibly losing other open unsaved documents. But IMO it is better to crash, 
which can be easily noticed, and losed unsaved documents, which can be 
crash-saved or autosaved (we do that, don't we?), rather than silently drop 
some big graph on page 87 that will be noticed only a week later.

 But ok, it's too much to just abort in every case. I think however that 
whether to abort or try to recover does not actually depend on the class 
where the problem occurs, but on where the class is used. I.e. I expect 
nobody will bother (=it's not worthwhile to do) to check for pixmap 
allocation failures and handle them somewhere in drawing code, but may check 
for them in an import filter. And even in the import filter, I think the only 
good reaction is bailing out completely and returning an error, not ignoring 
it silently like in the case I pointed out earlier. So we can split our 
source in two areas:
- the default, where we don't bother checking and just abort, because it's not 
worthwhile to make the effort
- special areas, where we expect allocation failures to be more likely (import 
filters, expensive operations in Calc, whatever), there we try to recover

 Which brings me to the following idea: We make rtl_allocateMemory() just 
abort by default. Those special areas get surrounded by setting a flag that 
will change that to throwing an exception, and each of such areas will be 
also inside a try/catch block that'll catch the exception and propagate 
failure of that code in a suitable way.

 That raises some question, but I think that's doable. The flag needs to be 
thread-safe, so I think it should be a mutex-protected 
this-thread-now-wants-exceptions. It'd also mean that our rtl C code would 
need to handle exceptions, but that should be binary compatible, and only 
code expecting this new behavior from rtl code would actually get it, so it 
should be safe. Does that make sense?

-- 
 Lubos Lunak
 l.lunak@suse.cz