checked sources of xorg-server 1.7.0 with static code analysis tool cppcheck
mcree at orcon.net.nz
Mon Oct 5 13:56:14 PDT 2009
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.
More information about the xorg-devel