checked sources of xorg-server 1.7.0 with static code analysis tool cppcheck

[Michael Cree]

On 6/10/2009, at 9:09 AM, Ping wrote:

> On Oct 3, 2009, at 03:53, Michel Dänzer wrote:
>
> On Fri, 2009-10-02 at 18:25 -0700, Jeremy Huddleston wrote:
> On Oct 2, 2009, at 16:17, Michel Dänzer wrote:
>
> On Fri, 2009-10-02 at 23:10 +0200, Martin Ettl wrote:
>
> diff --git a/exa/exa_classic.c b/exa/exa_classic.c
> index 1eff570..c9c7534 100644
> --- a/exa/exa_classic.c
> +++ b/exa/exa_classic.c
> @@ -144,14 +144,14 @@ Bool
> exaModifyPixmapHeader_classic(PixmapPtr pPixmap, int width, int
> height, int depth,
>                    int bitsPerPixel, int devKind, pointer pPixData)
> {
> +    if (!pPixmap)
> +        return FALSE;
> +
>   ScreenPtr pScreen = pPixmap->drawable.pScreen;
>   ExaScreenPrivPtr pExaScr;
>   ExaPixmapPrivPtr pExaPixmap;
>   Bool ret;
>
> -    if (!pPixmap)
> -        return FALSE;
> -
>   pExaScr = ExaGetScreenPriv(pScreen);
>   pExaPixmap = ExaGetPixmapPriv(pPixmap);
>
> [...]
> What purpose is that? If these functions were actually called with a
> NULL PixmapPtr, surely the current code would have crashed with a
> segmentation fault.
>
> My experience is "don't change it unless you know it is broken".   
> Gracefully returning a warning (FALSE) is better than making  
> yourself (xserver) look bad (crashing).

Be aware that the gcc compiler will remove the statement "if (! 
pPixmap) return FALSE;" in the original version if optimisation is  
turned on as the dereference of pPixmap to initialise pScreen implies  
that the pointer cannot be NULL.  Gcc takes the access via the pointer  
as a clear indication that the check for NULL is superfluous and  
optimises the test for NULL away.  The upshot is that without the  
patch applied, and using gcc with optimisation turned on, there is  
_no_ check whatsoever for the NULL pointer.

The person who said "If these functions were actually called with a  
NULL PixmapPtr, surely the current code would have crashed with  
segmentation fault" is only correct some of the time.  It is possible  
(particularly with Linux kernels more than a couple or so months old)  
to map memory at location 0 (which is typically the value of NULL)  
thus a segmentation violation would not occur.  Admittedly enabling  
memory maps at location 0 is not what the normal user does (with very  
few exceptions) but it is what a malicious user might do.

This coding practice (checking for NULL after dereferencing) appeared  
in the Linux kernel and the behaviour of gcc of optimising the check  
for NULL away formed a step in a published, and very ingenious, root  
exploit of the Linux kernel.

Cheers
Michael.