Bug 99451 - polygon offset use after free
Summary: polygon offset use after free
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Drivers/Gallium/radeonsi (show other bugs)
Version: git
Hardware: Other Linux (All)
: medium normal
Assignee: Default DRI bug account
QA Contact: Default DRI bug account
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-01-18 17:28 UTC by Zachary Michaels
Modified: 2017-01-19 09:53 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
gdb script to verify use after free (717 bytes, text/plain)
2017-01-18 17:28 UTC, Zachary Michaels
Details
potential fix (697 bytes, patch)
2017-01-18 17:30 UTC, Zachary Michaels
Details | Splinter Review
proposed fix (1.05 KB, patch)
2017-01-18 18:13 UTC, Zachary Michaels
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Zachary Michaels 2017-01-18 17:28:38 UTC
Created attachment 129030 [details]
gdb script to verify use after free

Our application enables and disables GL_POLYGON_OFFSET_FILL multiple times per frame, and it has been crashing on Ubuntu 16.04. (We are aware that our usage pattern is probably not ideal.) We believe the crash is occurring because the radeonsi driver is using the memory pointed to by si_context->queued.named.poly_offset after it has been freed.

We have verified the use after free behavior by running the attached gdb script against the master branch (commit 1e1bddf15a1720917b11e44dc639351ad613c3dc). Unfortunately we are not yet able to provide a sample application to run this against.

The following scenario may not be completely accurate, but hopefully it should give a feel for the sequence of events leading up to this issue:

glEnable(GL_POLYGON_OFFSET_FILL)
  * sets pipe_rasterizer_state->offset_tri to true
si_create_rs_state
  * callocs rasterizer A
  * sets A->uses_poly_offset to true because pipe_rasterizer_state->offset_tri is true
si_bind_rs_state
  * changes si_context->queued.named.rasterizer to rasterizer A
  * calls si_update_poly_offset_state to make queued.named.poly_offset point into rasterizer A
glDisable(GL_POLYGON_OFFSET_FILL)
  * sets pipe_rasterizer_state->offset_tri to false
si_create_rs_state
  * callocs rasterizer B
  * sets B->uses_poly_offset to false because pipe_rasterizer_state->offset_tri is false
si_bind_rs_state
  * changes the rasterizer to rasterizer B
  * calls si_update_poly_offset_state to make sure poly_offset is up to date
  * si_update_poly_offset_state
      * sets rs to si_context.queued.named.rasterizer, which is B
      * returns without updating poly_offset because B->uses_poly_offset is false
      * poly_offset still points into rasterizer A
si_delete_rs_state
  * does NOT set poly_offset to NULL because queued.named.rasterizer no longer points to rasterizer A
  * frees rasterizer A via si_pm4_delete_state
si_draw_vbo
  * calls si_pm4_emit_dirty
    * follows poly_offset into rasterizer A, which has been freed
    * bad things happen

The patch attached below ensures si_update_poly_offset sets poly_offset to NULL if uses_poly_offset is false. We think this makes sense because it always leaves poly_offset in a valid state. Either it points into the currently queued rasterizer, or it is NULL. If this does turn out to be the correct fix, the attempt to NULL poly_offset from si_delete_rs_state should probably be removed as well.

Thanks!
Comment 1 Zachary Michaels 2017-01-18 17:30:32 UTC
Created attachment 129031 [details] [review]
potential fix
Comment 2 Zachary Michaels 2017-01-18 18:13:49 UTC
Created attachment 129032 [details] [review]
proposed fix
Comment 3 Nicolai Hähnle 2017-01-19 09:53:53 UTC
Thanks for the clear report and fix. I've cleaned up the commit message slightly and pushed it to master, commit d7d32b3bfe86bd89d94d59393907bce1cb9dab7c.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct.