glamor-egl, parameter checking

[Zhigang Gong]

On Wed, Sep 25, 2013 at 06:29:07PM -0700, Seth Arnold wrote:
> Hello, I recently gave glamor-egl a very quick audit, and found some
> issues that I wanted to run past someone who would know the code better
> than I do.
> 
> - _glamor_poly_lines():
>   - unchecked malloc() return value
>   - if n = 1 is passed to the function, the malloc might allocate zero
>     bytes:
>             rects = malloc(sizeof(xRectangle) * (n - 1));
>   Can n = 1 be a realistic input to this function? It appears to be
>   callable from outside the library with arbitrary inputs.
>   Is this safe?

This function is to draw lines with at lest two points. So n = 1 is not a
realisitic input.

> 
> - _pixman_region_init_clipped_rectangles()
>   - The unsigned int num_rects argument is passed, unchecked, to
>     boxes = malloc(sizeof(pixman_box16_t) * num_rects);
>     -- can a large-enough value of num_rects cause a multiplication
>     overflow here, allocating less memory than necessary?
>   It appears to be callable from outside the library with arbitrary
>   inputs. Is this safe?

Theoretically, it is possible. But as it will only be called from DDX (Xserver) driver
internally. It should be checked at the first place when the xserver get the command request
from the client.

> 
> - glamor_create_composite_fs() appears to have two unguarded divisions
>   in relocate_texture that might result in divide-by-zero, wh.x and wh.y
>   -- are these guarded somewhere else?
> 
> - glamor_create_composite_fs() appears to have an unguarded division in
>   rel_sampler that might result in divide-by-zero, wh.xy -- is this
>   guarded somewhere else?

IMO, the shader thread will only be spawned(on GPU side) when it get a non-zero primitive
which means both the width/height should not be zero.

> 
> - glamor_pixmap_attach_fbo() has a switch statement that uses
>   fall-through after a block of code, but there's no comment nearby to
>   assure the reader that it is intentional. Is it intentional? :)
Right, it is intentional. A pixmap with a fbo attached should have a null buffer pointer
initially. And, you are right, there should be a comment to clarify that.

> 
> There are several unchecked memory allocations:
>   - glamor_compile_glsl_prog() unchecked malloc() return value
>   - glamor_egl_init() unchecked calloc() return value glamor_egl
>   - glamor_compute_clipped_regions_ext() unchecked calloc() return value
>     result_regions
>   - __glamor_compute_clipped_regions() unchecked calloc() return value
>     clipped_regions
>   - glamor_composite_largepixmap_region() unchecked malloc() return value
>     source_pixmap_priv
> 
> (While it's true malloc() will almost never return NULL, someone may
> someday wish to run this code with overcommit turned off, and it'd be
> better to be safe.)

You are right. We need to add some checking code here. I will do that latter.
Thanks.

-- Zhigang.

> 
> Thanks



> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel