OK I pushed a patch for this.
I believe not checking for NULL and still changing the internal state of
the image object led to this miserable situation.

2017-08-09 22:19 GMT+09:00 Carsten Haitzler <ras...@rasterman.com>:

> On Tue, 18 Jul 2017 21:57:23 -0700 Jean-Philippe ANDRÉ <j...@videolan.org>
> said:
>
> After much bisecting late into the evening... this patch broke
> enlightenment
> and makes it reliably crash when you run virtualbox... in texture
> uploading a
> seemingly garbage pointer in the image data...
>
> at least for now this is a note to "look into this" ... filing
>
> > jpeg pushed a commit to branch master.
> >
> > http://git.enlightenment.org/core/efl.git/commit/?id=
> 45c8e5e983d24b23b244513087799edcda862411
> >
> > commit 45c8e5e983d24b23b244513087799edcda862411
> > Author: Jean-Philippe Andre <jp.an...@samsung.com>
> > Date:   Wed Jul 19 11:43:35 2017 +0900
> >
> >     evas: Fix support for image_data_get on snapshot
> >
> >     evas_object_image_data_get() is a legacy API that I made work
> >     with snapshot objects (evas_object_image_snapshot_set()). Some
> >     changes in the engine broke the behaviour and this patch fixes
> >     it.
> >
> >     When getting the pixels from an FBO, in read-only mode, we need
> >     to create a temporary image (pixels surface) that contains the
> >     copy of the pixels we get from glReadPixels. This image needs
> >     to be deleted afterwards. It is thus stored by the image object
> >     and freed upon _image_data_set() (good) or object deletion (bad).
> >
> >     FBO + read-write is not supported by this API (it is supported
> >     through buffer_map as the filters had to use that).
> >
> >     Fixes T5754
> > ---
> >  src/lib/evas/canvas/evas_image_legacy.c  | 84
> ++++++++++++++++++++++++++
> > +----- src/lib/evas/canvas/evas_image_private.h |  1 +
> >  src/lib/evas/canvas/evas_object_image.c  |  7 +++
> >  3 files changed, 79 insertions(+), 13 deletions(-)
> >
> > diff --git a/src/lib/evas/canvas/evas_image_legacy.c
> > b/src/lib/evas/canvas/evas_image_legacy.c index a8593db29b..3f69dca4e6
> 100644
> > --- a/src/lib/evas/canvas/evas_image_legacy.c
> > +++ b/src/lib/evas/canvas/evas_image_legacy.c
> > @@ -14,6 +14,14 @@
> >     EVAS_IMAGE_API(_o, __VA_ARGS__); \
> >     } while (0)
> >
> > +typedef struct _Evas_Image_Legacy_Pixels_Entry
> > Evas_Image_Legacy_Pixels_Entry; +
> > +struct _Evas_Image_Legacy_Pixels_Entry
> > +{
> > +   Eo    *object;
> > +   void  *image;
> > +};
> > +
> >  EAPI Evas_Object *
> >  evas_object_image_add(Evas *eo_e)
> >  {
> > @@ -552,7 +560,7 @@ evas_object_image_data_set(Eo *eo_obj, void *data)
> >
> >     Evas_Object_Protected_Data *obj = efl_data_scope_get(eo_obj,
> > EFL_CANVAS_OBJECT_CLASS); Evas_Image_Data *o = efl_data_scope_get(eo_obj,
> > EFL_CANVAS_IMAGE_INTERNAL_CLASS);
> > -   void *p_data;
> > +   void *p_data, *pixels;
> >     Eina_Bool resize_call = EINA_FALSE;
> >
> >
> > @@ -563,6 +571,13 @@ evas_object_image_data_set(Eo *eo_obj, void *data)
> >     p_data = o->engine_data;
> >     if (data)
> >       {
> > +        // r/o FBO data_get: only free the image, don't update pixels
> > +        if ((pixels = eina_hash_find(o->pixels->images_to_free, data))
> !=
> > NULL)
> > +          {
> > +             eina_hash_del(o->pixels->images_to_free, data, pixels);
> > +             return;
> > +          }
> > +
> >          if (o->engine_data)
> >            {
> >               o->engine_data = ENFN->image_data_put(ENDT, o->engine_data,
> > data); @@ -640,14 +655,28 @@ evas_object_image_data_set(Eo *eo_obj, void
> > *data) if (resize_call) evas_object_inform_call_image_resize(eo_obj);
> >  }
> >
> > +static void
> > +_image_to_free_del_cb(void *data)
> > +{
> > +   Evas_Image_Legacy_Pixels_Entry *px_entry = data;
> > +   Evas_Object_Protected_Data *obj;
> > +
> > +   obj = efl_data_scope_safe_get(px_entry->object,
> EFL_CANVAS_OBJECT_CLASS);
> > +   EINA_SAFETY_ON_NULL_RETURN(obj);
> > +   ENFN->image_free(ENDT, px_entry->image);
> > +   free(px_entry);
> > +}
> > +
> >  EAPI void*
> >  evas_object_image_data_get(const Eo *eo_obj, Eina_Bool for_writing)
> >  {
> >     EVAS_IMAGE_API(eo_obj, NULL);
> >
> >     Evas_Image_Data *o = efl_data_scope_get(eo_obj,
> > EFL_CANVAS_IMAGE_INTERNAL_CLASS);
> > +   Evas_Image_Legacy_Pixels_Entry *px_entry = NULL;
> > +   Eina_Bool tofree = 0;
> > +   void *pixels = NULL;
> >     int stride = 0;
> > -   void *pixels;
> >     DATA32 *data;
> >
> >     if (!o->engine_data) return NULL;
> > @@ -662,25 +691,48 @@ evas_object_image_data_get(const Eo *eo_obj,
> Eina_Bool
> > for_writing) ENFN->image_scale_hint_set(ENDT, o->engine_data,
> o->scale_hint);
> >     if (ENFN->image_content_hint_set)
> >       ENFN->image_content_hint_set(ENDT, o->engine_data,
> o->content_hint);
> > -   pixels = ENFN->image_data_get(ENDT, o->engine_data, for_writing,
> &data,
> > &o->load_error, NULL);
> > +   pixels = ENFN->image_data_get(ENDT, o->engine_data, for_writing,
> &data,
> > &o->load_error, &tofree);
> >     /* if we fail to get engine_data, we have to return NULL */
> >     if (!pixels) return NULL;
> >
> > -   o->engine_data = pixels;
> > -   if (ENFN->image_stride_get)
> > -     ENFN->image_stride_get(ENDT, o->engine_data, &stride);
> > -   else
> > -     stride = o->cur->image.w * 4;
> > +   if (!tofree)
> > +     {
> > +        o->engine_data = pixels;
> > +        if (ENFN->image_stride_get)
> > +          ENFN->image_stride_get(ENDT, o->engine_data, &stride);
> > +        else
> > +           stride = o->cur->image.w * 4;
> >
> > -   if (o->cur->image.stride != stride)
> > +        if (o->cur->image.stride != stride)
> > +          {
> > +             EINA_COW_IMAGE_STATE_WRITE_BEGIN(o, state_write)
> > +                   state_write->image.stride = stride;
> > +             EINA_COW_IMAGE_STATE_WRITE_END(o, state_write);
> > +          }
> > +
> > +        o->pixels_checked_out++;
> > +     }
> > +   else
> >       {
> > -        EINA_COW_IMAGE_STATE_WRITE_BEGIN(o, state_write)
> > -          state_write->image.stride = stride;
> > -        EINA_COW_IMAGE_STATE_WRITE_END(o, state_write);
> > +        Eina_Hash *hash = o->pixels->images_to_free;
> > +
> > +        if (!hash)
> > +          {
> > +             hash = eina_hash_pointer_new(_image_to_free_del_cb);
> > +             if (!hash) goto error;
> > +             EINA_COW_PIXEL_WRITE_BEGIN(o, pixi_write)
> > +               pixi_write->images_to_free = hash;
> > +             EINA_COW_PIXEL_WRITE_END(o, pixi_write);
> > +          }
> > +
> > +        px_entry = calloc(1, sizeof(*px_entry));
> > +        px_entry->object = (Eo *) eo_obj;
> > +        px_entry->image = pixels;
> > +        if (!eina_hash_add(hash, data, px_entry))
> > +          goto error;
> >       }
> >
> > -   o->pixels_checked_out++;
> >     if (for_writing)
> >       {
> >          o->written = EINA_TRUE;
> > @@ -688,6 +740,12 @@ evas_object_image_data_get(const Eo *eo_obj,
> Eina_Bool
> > for_writing) }
> >
> >     return data;
> > +
> > +error:
> > +   free(px_entry);
> > +   if (tofree && pixels)
> > +     ENFN->image_free(ENDT, pixels);
> > +   return NULL;
> >  }
> >
> >  EAPI void
> > diff --git a/src/lib/evas/canvas/evas_image_private.h
> > b/src/lib/evas/canvas/evas_image_private.h index 67cfee41d1..91c7e22af1
> 100644
> > --- a/src/lib/evas/canvas/evas_image_private.h
> > +++ b/src/lib/evas/canvas/evas_image_private.h
> > @@ -61,6 +61,7 @@ struct _Evas_Object_Image_Pixels
> >        Evas_Object_Image_Pixels_Get_Cb  get_pixels;
> >        void                            *get_pixels_data;
> >     } func;
> > +   Eina_Hash       *images_to_free; /* pixel void* ->
> > Evas_Image_Legacy_Pixels_Entry */
> >     Evas_Video_Surface video;
> >     unsigned int video_caps;
> > diff --git a/src/lib/evas/canvas/evas_object_image.c
> > b/src/lib/evas/canvas/evas_object_image.c index c522742d7a..5506290ba6
> 100644
> > --- a/src/lib/evas/canvas/evas_object_image.c
> > +++ b/src/lib/evas/canvas/evas_object_image.c
> > @@ -1511,6 +1511,13 @@ evas_object_image_free(Evas_Object *eo_obj,
> > Evas_Object_Protected_Data *obj) efl_del(o->file_obj);
> >          o->file_obj = NULL;
> >       }
> > +   if (o->pixels->images_to_free)
> > +     {
> > +        eina_hash_free(o->pixels->images_to_free);
> > +        EINA_COW_PIXEL_WRITE_BEGIN(o, pixi_write)
> > +          pixi_write->images_to_free = NULL;
> > +        EINA_COW_PIXEL_WRITE_END(o, pixi_write);
> > +     }
> >     if (o->pixels->pixel_updates)
> >       {
> >         EINA_COW_PIXEL_WRITE_BEGIN(o, pixi_write)
> >
> > --
> >
> >
>
>
> --
> ------------- Codito, ergo sum - "I code, therefore I am" --------------
> The Rasterman (Carsten Haitzler)    ras...@rasterman.com
>
>


-- 
Jean-Philippe André
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel

Reply via email to