Am Montag, den 03.09.2018, 10:52 +0100 schrieb Emil Velikov:
> Hi Gert,
>
> On 3 September 2018 at 09:17, Gert Wollny <gert.wol...@collabora.com>
> wrote:
> > The ressource storage must already be resized when the count is
> > equal to
> > the allocated size and the space for the handles must be resized
> > accordingly.
> >
>
> s/ressource/resource/
>
> > Fixes:
> > Crashes with
> > piglit/bin/map_buffer_range-invalidate CopyBufferSubData \
> > increment-offset -auto -fbo
> >
> > Invalid write of size 8
> > at 0xB72E4CF: virgl_drm_add_res (virgl_drm_winsys.c:629)
> > by 0xB72E4CF: virgl_drm_emit_res (virgl_drm_winsys.c:663)
> > by 0xB72A44A: virgl_encode_resource_copy_region
> > (virgl_encode.c:776)
> > by 0xB40CD12: st_copy_buffer_subdata (st_cb_bufferobjects.c:585)
> > by 0xB244A3B: _mesa_CopyBufferSubData (bufferobj.c:2940)
> > by 0x109A1E: upload (invalidate.c:169)
> > by 0x109C2F: piglit_display (invalidate.c:215)
> > by 0x4F80FBE: run_test (piglit_fbo_framework.c:52)
> > by 0x4F66E5F: piglit_gl_test_run (piglit-framework-gl.c:229)
> > by 0x10949D: main (invalidate.c:47)
> > Address 0xbe07d30 is 0 bytes after a block of size 4,096 alloc'd
> > at 0x4C31B25: calloc (in /usr/lib/valgrind/vgpreload_memcheck-
> > amd64-
> > linux.so)
> > by 0xB72DAAF: virgl_drm_cmd_buf_create (virgl_drm_winsys.c:567)
> >
> > and
> >
> > Invalid write of size 4
> > at 0xB72E4F0: virgl_drm_add_res (virgl_drm_winsys.c:631)
> > by 0xB72E4F0: virgl_drm_emit_res (virgl_drm_winsys.c:663)
> > by 0xB72A44A: virgl_encode_resource_copy_region
> > (virgl_encode.c:776)
> > by 0xB40CD12: st_copy_buffer_subdata (st_cb_bufferobjects.c:585)
> > by 0xB244A3B: _mesa_CopyBufferSubData (bufferobj.c:2940)
> > by 0x109A1E: upload (invalidate.c:169)
> > by 0x109C2F: piglit_display (invalidate.c:215)
> > by 0x4F80FBE: run_test (piglit_fbo_framework.c:52)
> > by 0x4F66E5F: piglit_gl_test_run (piglit-framework-gl.c:229)
> > by 0x10949D: main (invalidate.c:47)
> > Address 0xbe08570 is 0 bytes after a block of size 2,048 alloc'd
> > at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-
> > amd64-
> > linux.so)
> > by 0xB72DAC8: virgl_drm_cmd_buf_create (virgl_drm_winsys.c:572)
> >
> > Signed-off-by: Gert Wollny <gert.wol...@collabora.com>
> > ---
> > src/gallium/winsys/virgl/drm/virgl_drm_winsys.c | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
> > b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
> > index aad6430c41..2976b46484 100644
> > --- a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
> > +++ b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
> > @@ -617,13 +617,18 @@ static void virgl_drm_add_res(struct
> > virgl_drm_winsys *qdws,
> > {
> > unsigned hash = res->res_handle & (sizeof(cbuf-
> > >is_handle_added)-1);
> >
> > - if (cbuf->cres > cbuf->nres) {
> > + if (cbuf->cres >= cbuf->nres) {
>
> I'd leave the check as-is.
But it's not correct, the access cbuf->res_bo[cbuf->cres] is past the
end when (cbuf->cres == cbuf->nres). Maybe I should put it in a
separate patch to make this clearer (i.e. the first valgrind trace is
because od the ">" and the second because res_hlist is not re-
allocated.
>
> > cbuf->nres += 256;
> > cbuf->res_bo = realloc(cbuf->res_bo, cbuf->nres *
> > sizeof(struct virgl_hw_buf*));
> > if (!cbuf->res_bo) {
> > fprintf(stderr,"failure to add relocation %d, %d\n",
> > cbuf->cres, cbuf->nres);
> > return;
> > }
> > + cbuf->res_hlist = realloc(cbuf->res_hlist, cbuf->nres *
> > sizeof(uint32_t));
> > + if (!cbuf->res_hlist) {
> > + fprintf(stderr,"failure to add hlist relocation %d,
> > %d\n", cbuf->cres, cbuf->nres);
> > + return;
> > + }
>
> Please use the uppercase REALLOC wrapper and do not leak memory of
> the call fails.
That's for copy-pasting, will update the patch.
> As a follow-up worth applying the same nitpicks to vtest?
I'll have a look, Tomeu already run into problems with the handles
there, it might be implemented differently.
best,
Gert
>
> With the conditional and REALLOC (feel free to keep the leak fix as
> follow-up)
>
> Fixes: 4b15b5e803e ("virgl: resize resource bo allocation if we need
> to.")
> Reviewed-by: Emil Velikov <emil.veli...@collabora.com>
>
> -Emil
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
