Page MenuHomePhabricator

Evas text: set NULL free'd pointers in evas_object_text_free()
ClosedPublic

Authored by id213sin on Dec 13 2015, 11:38 PM.

Details

Summary

_render_pre() function could be called for an object which is
going to be deleted. According to state changes of the object,
text could be recalculated with free'd pointers. It caused an
invalid read and crash.
@fix

Test Plan
  1. Apply D1747.
  2. Run elementary_test.
  3. Put any character in elm_entry and change paragraph direction.
  4. Put any character again.
  5. It can cause a crash which is caused by invalid read in Evas Text.

Diff Detail

Repository
rEFL core/efl
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
id213sin updated this revision to Diff 7815.Dec 13 2015, 11:38 PM
id213sin retitled this revision from to Evas: Don't check the paragraph direction if it is going to be deleted..
id213sin updated this object.
id213sin edited the test plan for this revision. (Show Details)
id213sin added reviewers: tasn, herdsman, woohyun.
tasn edited edge metadata.Dec 14 2015, 3:47 AM

While I'm sure this fixes an issue you're facing, I need some help from you with understanding it.

My problem is that this is a path I don't think is very good to take, that is, we don't check obj->delete_me for anything else, we never needed to, so why do we need to do it here in this case? Why is this code worse (different?) in that regard, that it needs this extra check?

@tasn
Actually, I want some advices about this invalid read issue.
I also think it is not good patch for the issue.
There are two major points about this.

  1. *_render_pre() function is called for deleted object [obj->delete_me > 0].
  2. evas_object_text_free() does not care about free'd pointer.

If *_render_pre() function could be called for deleted object and it is normal case,
we can prevent invalid read by setting NULL all of free'd pointers.
Do you have any idea about render logic?

tasn added a subscriber: raster.Dec 21 2015, 7:06 AM

To be honest, I don't remember. I think calling _render_pre() on an about-to-be-deleted object is valid, as we keep the objects around for a few frames. Not sure what the correct behaviour should be though.

@raster, do you have any insights? Please focus on the comments and not the patch itself.

id213sin removed a subscriber: raster.
id213sin updated this revision to Diff 8437.Feb 14 2016, 11:45 PM

Revert previous changes and set NULL to free'd pointers in Evas Text.

Commit message will be changed.

id213sin retitled this revision from Evas: Don't check the paragraph direction if it is going to be deleted. to Evas text: set NULL free'd pointers in evas_object_text_free().Feb 14 2016, 11:46 PM
id213sin updated this object.

As raster's opinion on IRC, update the patch to set NULL to free'd pointers.

This revision was automatically updated to reflect the committed changes.