[vlc-devel] [VLC] #2366: realloc is generally used incorrectly

[Rémi Denis-Courmont]

Le lundi 25 mai 2009 21:55:02 Laurent Aimar, vous avez écrit :
> On Mon, May 25, 2009, Rémi Denis-Courmont wrote:
> > A demand-paging operating system will hit OOM when memory is accessed,
> > _not_ when memory is allocated. The C language provides no (portable) way
> > to handle errors during memory accesses. Checking for realloc() errors
> > will not fix out- of-memory handling; it would however fix
> > out-of-address-space handling (i.e. ridiculously large allocation
> > requests), if and only if we were to fix malloc() and calloc() as well,
> > including in all underlying libraries.
>
>  I wonder if we should not take a more pragmatic approche:
>  [a] check for malloc/calloc/realloc everytime the memory needed is known
> to be potentially large or value comming from untrusted source (files, net,
> ...).
> [b] do not check for known small amount (like strings, small
> structures, etc)

Assuming that we cannot:
 * detect out-of-memory (due to demand-paging),
 * detect out-of-address space (due to underlying libraries),
 * do much "good" even when we detect either of those,
I think crashing the process is just fine, for lack of a better alternative.

From the security perspective, a really bad problem occurs if there is a 
"large" write offset into the allocated memory. If we just write at 0x0 or 
close, then a segmentation fault will occur (so long as page zero is not 
mapped, which would imply we were hacked already).
Indeed NULL + offset = offset = evil things possible.

(Integer overflow into the allocation size are another issue by the way.)

>  The code path from a malloc failure is in practice never tested, and so
> will be full of bugs. There is no way to escape that. Limiting the
> attention to where it is really dangerous (or more probable) will probably
> make a better code and ease dev and so increase code quality.

On a high-level, I can only agree. But I don't know how to "decide" this on a 
low-level. Perhaps the simplest yet is to do if (NULL) abort(); wherever we do 
an allocation yet cannot be bothered to implement proper error handling. We 
already do exactly that for mutexes and condition variables.


In any case, process separation still looks like the only way to protect the 
browser from the VLC plugin.

-- 
Rémi Denis-Courmont
http://www.remlab.net/