[PATCH xserver v8 05/14] modesetting: Use atomic modesetting API for pageflip if available

[Adam Jackson]

On Wed, 2018-09-12 at 11:01 +1000, Dave Airlie wrote:
> On Wed, 28 Feb 2018 at 11:21, Daniel Stone <daniels at collabora.com> wrote:
> > 
> > +static Bool
> > +drmmode_prop_info_copy(drmmode_prop_info_ptr dst,
> > +                      const drmmode_prop_info_rec *src,
> > +                      unsigned int num_props,
> > +                      Bool copy_prop_id)
> > +{
> > +    unsigned int i;
> > +
> > +    memcpy(dst, src, num_props * sizeof(*dst));
> > +
> > +    for (i = 0; i < num_props; i++) {
> > +        unsigned int j;
> > +
> > +        if (copy_prop_id)
> > +            dst[i].prop_id = src[i].prop_id;
> > +        else
> > +            dst[i].prop_id = 0;
> > +
> > +        if (src[i].num_enum_values == 0)
> > +            continue;
> > +
> > +        dst[i].enum_values =
> > +            malloc(src[i].num_enum_values *
> > +                    sizeof(*dst[i].enum_values));
> > +        if (!dst[i].enum_values)
> > +            goto err;
> > +
> > +        memcpy(dst[i].enum_values, src[i].enum_values,
> > +                src[i].num_enum_values * sizeof(*dst[i].enum_values));
> > +
> > +        for (j = 0; j < dst[i].num_enum_values; j++)
> > +            dst[i].enum_values[j].valid = FALSE;
> > +    }
> > +
> > +    return TRUE;
> > +
> > +err:
> > +    while (i--)
> > +        free(dst[i].enum_values);
> > +    free(dst);
> > +    return FALSE;
> > +}
> 
> On failure this frees dst, however...
> 
> > +drmmode_crtc_create_planes(xf86CrtcPtr crtc, int num)
> > +{
> > +    drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
> > +    drmmode_ptr drmmode = drmmode_crtc->drmmode;
> > +    drmModePlaneRes *kplane_res;
> > +    drmModePlane *kplane;
> > +    drmModeObjectProperties *props;
> > +    uint32_t i, type;
> > +    int current_crtc, best_plane = 0;
> > +
> > +    static drmmode_prop_enum_info_rec plane_type_enums[] = {
> > +        [DRMMODE_PLANE_TYPE_PRIMARY] = {
> > +            .name = "Primary",
> > +        },
> > +        [DRMMODE_PLANE_TYPE_OVERLAY] = {
> > +            .name = "Overlay",
> > +        },
> > +        [DRMMODE_PLANE_TYPE_CURSOR] = {
> > +            .name = "Cursor",
> > +        },
> > +    };
> > +    static const drmmode_prop_info_rec plane_props[] = {
> > +        [DRMMODE_PLANE_TYPE] = {
> > +            .name = "type",
> > +            .enum_values = plane_type_enums,
> > +            .num_enum_values = DRMMODE_PLANE_TYPE__COUNT,
> > +        },
> > +        [DRMMODE_PLANE_FB_ID] = { .name = "FB_ID", },
> > +        [DRMMODE_PLANE_CRTC_ID] = { .name = "CRTC_ID", },
> > +        [DRMMODE_PLANE_SRC_X] = { .name = "SRC_X", },
> > +        [DRMMODE_PLANE_SRC_Y] = { .name = "SRC_Y", },
> > +    };
> > +    drmmode_prop_info_rec tmp_props[DRMMODE_PLANE__COUNT];
> 
> We use a stack allocated tmp_props here.

AFAICT the free(dst) is always wrong. The other places we use
drmmode_prop_info_copy, we're initializing an array in the middle of a
drmmode_{crtc,output}_private_rec.

- ajax