On Thu, 10 Aug 2017 11:57:10 +0900 Jean-Philippe André <j...@videolan.org> said:

yay!

> 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


-- 
------------- Codito, ergo sum - "I code, therefore I am" --------------
The Rasterman (Carsten Haitzler)    ras...@rasterman.com


------------------------------------------------------------------------------
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