diff mbox

[i-g-t] tests/gem_storedw_loop: Fix use after free for bufmgr

Message ID 1442944959-29467-1-git-send-email-robert.beckett@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

robert.beckett@intel.com Sept. 22, 2015, 6:02 p.m. UTC
From: Robert Beckett <robert.beckett@intel.com>

Move bufmgr destruction to end of tests. This avoids use after free on
successive tests.

This could be squashed with the following patch to aid bisectability:

commit 0679702150157706a6def66b893b29c16345f4db
Author: Jesse Barnes <jbarnes@virtuousgeek.org>
Date:   Wed Aug 5 16:06:31 2015 -0700

    tests/gem_storedw_loop: add new store_dword test to unify per-ring ones v2

Signed-off-by: Robert Beckett <robert.beckett@intel.com>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 tests/gem_storedw_loop.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jesse Barnes Sept. 22, 2015, 7:08 p.m. UTC | #1
On 09/22/2015 11:02 AM, robert.beckett@intel.com wrote:
> From: Robert Beckett <robert.beckett@intel.com>
> 
> Move bufmgr destruction to end of tests. This avoids use after free on
> successive tests.
> 
> This could be squashed with the following patch to aid bisectability:
> 
> commit 0679702150157706a6def66b893b29c16345f4db
> Author: Jesse Barnes <jbarnes@virtuousgeek.org>
> Date:   Wed Aug 5 16:06:31 2015 -0700
> 
>     tests/gem_storedw_loop: add new store_dword test to unify per-ring ones v2
> 
> Signed-off-by: Robert Beckett <robert.beckett@intel.com>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  tests/gem_storedw_loop.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/gem_storedw_loop.c b/tests/gem_storedw_loop.c
> index 10b12d4..e7ebcc2 100644
> --- a/tests/gem_storedw_loop.c
> +++ b/tests/gem_storedw_loop.c
> @@ -128,7 +128,6 @@ store_test(int ring, int count)
>  
>  	drm_intel_bo_unreference(target_buffer);
>  	intel_batchbuffer_free(batch);
> -	drm_intel_bufmgr_destroy(bufmgr);
>  }
>  
>  struct ring {
> @@ -181,5 +180,6 @@ igt_main
>  		}
>  	}
>  
> +	drm_intel_bufmgr_destroy(bufmgr);
>  	close(fd);
>  }
> 

Looks good, thanks.  Unfortunately too late to squash...

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Jesse
Daniel Vetter Sept. 23, 2015, 4:01 p.m. UTC | #2
On Tue, Sep 22, 2015 at 12:08:05PM -0700, Jesse Barnes wrote:
> On 09/22/2015 11:02 AM, robert.beckett@intel.com wrote:
> > From: Robert Beckett <robert.beckett@intel.com>
> > 
> > Move bufmgr destruction to end of tests. This avoids use after free on
> > successive tests.
> > 
> > This could be squashed with the following patch to aid bisectability:
> > 
> > commit 0679702150157706a6def66b893b29c16345f4db
> > Author: Jesse Barnes <jbarnes@virtuousgeek.org>
> > Date:   Wed Aug 5 16:06:31 2015 -0700
> > 
> >     tests/gem_storedw_loop: add new store_dword test to unify per-ring ones v2
> > 
> > Signed-off-by: Robert Beckett <robert.beckett@intel.com>
> > Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  tests/gem_storedw_loop.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tests/gem_storedw_loop.c b/tests/gem_storedw_loop.c
> > index 10b12d4..e7ebcc2 100644
> > --- a/tests/gem_storedw_loop.c
> > +++ b/tests/gem_storedw_loop.c
> > @@ -128,7 +128,6 @@ store_test(int ring, int count)
> >  
> >  	drm_intel_bo_unreference(target_buffer);
> >  	intel_batchbuffer_free(batch);
> > -	drm_intel_bufmgr_destroy(bufmgr);
> >  }
> >  
> >  struct ring {
> > @@ -181,5 +180,6 @@ igt_main
> >  		}
> >  	}
> >  
> > +	drm_intel_bufmgr_destroy(bufmgr);
> >  	close(fd);
> >  }
> > 
> 
> Looks good, thanks.  Unfortunately too late to squash...
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

No one from vpg has igt commit access so you need to push patches for
them. I've done that now.
-Daniel
Thomas Wood Sept. 23, 2015, 4:27 p.m. UTC | #3
On 23 September 2015 at 17:01, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Sep 22, 2015 at 12:08:05PM -0700, Jesse Barnes wrote:
>> On 09/22/2015 11:02 AM, robert.beckett@intel.com wrote:
>> > From: Robert Beckett <robert.beckett@intel.com>
>> >
>> > Move bufmgr destruction to end of tests. This avoids use after free on
>> > successive tests.
>> >
>> > This could be squashed with the following patch to aid bisectability:
>> >
>> > commit 0679702150157706a6def66b893b29c16345f4db
>> > Author: Jesse Barnes <jbarnes@virtuousgeek.org>
>> > Date:   Wed Aug 5 16:06:31 2015 -0700
>> >
>> >     tests/gem_storedw_loop: add new store_dword test to unify per-ring ones v2
>> >
>> > Signed-off-by: Robert Beckett <robert.beckett@intel.com>
>> > Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
>> > ---
>> >  tests/gem_storedw_loop.c |    2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/tests/gem_storedw_loop.c b/tests/gem_storedw_loop.c
>> > index 10b12d4..e7ebcc2 100644
>> > --- a/tests/gem_storedw_loop.c
>> > +++ b/tests/gem_storedw_loop.c
>> > @@ -128,7 +128,6 @@ store_test(int ring, int count)
>> >
>> >     drm_intel_bo_unreference(target_buffer);
>> >     intel_batchbuffer_free(batch);
>> > -   drm_intel_bufmgr_destroy(bufmgr);
>> >  }
>> >
>> >  struct ring {
>> > @@ -181,5 +180,6 @@ igt_main
>> >             }
>> >     }
>> >
>> > +   drm_intel_bufmgr_destroy(bufmgr);
>> >     close(fd);
>> >  }
>> >
>>
>> Looks good, thanks.  Unfortunately too late to squash...
>>
>> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>
> No one from vpg has igt commit access so you need to push patches for
> them. I've done that now.

Unfortunately this patch causes --list-subtest to fail, since the call
to drm_intel_bufmgr_destroy is called with NULL which causes a
segmentation fault. The calls to drm_intel_bufmgr_destroy and close
should be in an igt_fixture block.


> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

diff --git a/tests/gem_storedw_loop.c b/tests/gem_storedw_loop.c
index 10b12d4..e7ebcc2 100644
--- a/tests/gem_storedw_loop.c
+++ b/tests/gem_storedw_loop.c
@@ -128,7 +128,6 @@  store_test(int ring, int count)
 
 	drm_intel_bo_unreference(target_buffer);
 	intel_batchbuffer_free(batch);
-	drm_intel_bufmgr_destroy(bufmgr);
 }
 
 struct ring {
@@ -181,5 +180,6 @@  igt_main
 		}
 	}
 
+	drm_intel_bufmgr_destroy(bufmgr);
 	close(fd);
 }