On 3 September 2018 at 13:57, Gert Wollny <gert.wol...@collabora.com> wrote:
> Fixes crash with
> piglit/bin/map_buffer_range-invalidate CopyBufferSubData \
> increment-offset -auto -fbo
>
> * Resize the resource storage already when the count is equal to the
> allocated size, fixes:
>
> 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)
>
> * Also resize the space allocated for the handles, fixes:
>
> 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)
>
> Fixes: 4b15b5e803e ("virgl: resize resource bo allocation if we need to.")
>
> v2: - Use REALLOC macro and avoid memory leak when re-allocation fails
> - add Fixes tag (both Emil Velikov)
> - reorder commit message
>
> Signed-off-by: Gert Wollny <gert.wol...@collabora.com>
> Reviewed-by: Emil Velikov <emil.veli...@collabora.com> (v1)
>
> ---
> I resend it because I didn't apply all the changes Emil suggested,
> specifically
> I still change to compare for equality. The new commit message should make it
> clearer what this fixes.
>
Ack. I've throws some nitpicks below, please don't read too much into them.
Earlier R-B still stands.
> .../winsys/virgl/drm/virgl_drm_winsys.c | 24 +++++++++++++++----
> 1 file changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
> b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
> index aad6430c41..e780a5e514 100644
> --- a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
> +++ b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
> @@ -617,13 +617,27 @@ 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) {
> - 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);
> + if (cbuf->cres >= cbuf->nres) {
> + unsigned new_nres = cbuf->nres + 256;
> + void *new_ptr = REALLOC(cbuf->res_bo, cbuf->nres * sizeof(struct
> virgl_hw_buf*),
struct virgl_hw_res **new_bo =
> + new_nres * sizeof(struct virgl_hw_buf*));
> + if (!new_ptr) {
> + fprintf(stderr,"failure to add relocation %d, %d\n", cbuf->cres,
> new_nres);
> return;
> }
> + cbuf->res_bo = new_ptr;
> +
> + new_ptr = REALLOC(cbuf->res_hlist, cbuf->nres * sizeof(uint32_t),
uint32_t *new_hlist =
> + new_nres * sizeof(uint32_t));
> + if (!new_ptr) {
> + fprintf(stderr,"failure to add hlist relocation %d, %d\n",
> cbuf->cres, cbuf->nres);
FREE(new_bo);
> + return;
> + }
> + cbuf->res_hlist = new_ptr;
> + /* This is not perfect, because when the first re-allocation succeeds
> but
> + * the second failes the first old_size will be wrong, but currently it
> + * isn't used anyway. */
Now you can drop the comment.
Aside: the old_size of REALLOC could/should go - debug_realloc can use
old_hdr->size.
Using sizeof(instance) should be clearer and more consistent with the
rest of winsys/virgl.
-Emil
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
