[v1,2/2] test: fix resources usage for depthstencil-render-miplevels

Submitted by Sergii Romantsov on Nov. 21, 2018, 10:18 a.m.

Details

Message ID 1542795533-19793-2-git-send-email-sergii.romantsov@globallogic.com
State New
Headers show
Series "Series without cover letter" ( rev: 2 1 ) in Piglit

Not browsing as part of any series.

Commit Message

Sergii Romantsov Nov. 21, 2018, 10:18 a.m.
Usage test 'depthstencil-render-miplevels 200 s=z24_s8' causes core dump on exit.
Issues:
1. Allocation of memory many times to the same variable
2. Not complete array to store pointers
3. Absence of memory freeing

CC: Eric Anholt <eric@anholt.net>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108819
Fixes: 7a0e61d7792f (depthstencil-render-miplevels: Present the results in non-auto mode.)
Signed-off-by: Sergii Romantsov <sergii.romantsov@globallogic.com>
---
 tests/texturing/depthstencil-render-miplevels.cpp | 37 ++++++++++++++++++-----
 1 file changed, 30 insertions(+), 7 deletions(-)

Patch hide | download patch | download mbox

diff --git a/tests/texturing/depthstencil-render-miplevels.cpp b/tests/texturing/depthstencil-render-miplevels.cpp
index 87af92f..f3b1d02 100644
--- a/tests/texturing/depthstencil-render-miplevels.cpp
+++ b/tests/texturing/depthstencil-render-miplevels.cpp
@@ -86,6 +86,9 @@ 
 
 #include "piglit-util-gl.h"
 
+extern "C" void
+piglit_deinit(void);
+
 PIGLIT_GL_TEST_CONFIG_BEGIN
 
 	config.supports_gl_compat_version = 10;
@@ -94,6 +97,7 @@  PIGLIT_GL_TEST_CONFIG_BEGIN
 	config.window_height = 512;
 	config.window_visual = PIGLIT_GL_VISUAL_RGB | PIGLIT_GL_VISUAL_DOUBLE;
 	config.khr_no_error_support = PIGLIT_NO_ERRORS;
+	config.deinit = piglit_deinit;
 
 PIGLIT_GL_TEST_CONFIG_END
 
@@ -109,9 +113,28 @@  bool attach_together = false;
 bool attach_stencil_first = false;
 GLenum depth_format;
 int miplevel0_size;
-int max_miplevel;
-float **depth_miplevel_data;
-uint8_t **stencil_miplevel_data;
+int max_miplevel = 0;
+float **depth_miplevel_data = NULL;
+uint8_t **stencil_miplevel_data = NULL;
+
+extern "C" void
+piglit_deinit()
+{
+	if (depth_miplevel_data)
+	{
+		for (int i = 0; i <= max_miplevel; ++i)
+			if (depth_miplevel_data[i])
+				free(depth_miplevel_data[i]);
+		free(depth_miplevel_data);
+	}
+	if (stencil_miplevel_data)
+	{
+		for (int i = 0; i <= max_miplevel; ++i)
+			if (stencil_miplevel_data[i])
+				free(stencil_miplevel_data[i]);
+		free(stencil_miplevel_data);
+	}
+}
 
 /**
  * Check if the given depth/stencil/rgba texture internal format is supported.
@@ -279,7 +302,7 @@  test_miplevel(int level)
 
 		if (!piglit_automatic) {
 			depth_miplevel_data[level] =
-				(float *)malloc(4 * dim * dim);
+				(float *)realloc(depth_miplevel_data[level], 4 * dim * dim);
 			glReadPixels(0, 0, dim, dim,
 				     GL_DEPTH_COMPONENT, GL_FLOAT,
 				     depth_miplevel_data[level]);
@@ -294,7 +317,7 @@  test_miplevel(int level)
 
 		if (!piglit_automatic) {
 			stencil_miplevel_data[level] =
-				(uint8_t *)malloc(dim * dim);
+				(uint8_t *)realloc(stencil_miplevel_data[level], dim * dim);
 			glReadPixels(0, 0, dim, dim,
 				     GL_STENCIL_INDEX, GL_UNSIGNED_BYTE,
 				     stencil_miplevel_data[level]);
@@ -360,8 +383,8 @@  piglit_init(int argc, char **argv)
 	piglit_require_extension("GL_ARB_depth_texture");
 	piglit_require_extension("GL_ARB_texture_non_power_of_two");
 
-	depth_miplevel_data = (float **)calloc(max_miplevel, sizeof(float *));
-	stencil_miplevel_data = (uint8_t **)calloc(max_miplevel,
+	depth_miplevel_data = (float **)calloc(max_miplevel + 1, sizeof(float *));
+	stencil_miplevel_data = (uint8_t **)calloc(max_miplevel + 1,
 						   sizeof(uint8_t *));
 
 	/* argv[2]: buffer combination */

Comments

Sergii Romantsov <sergii.romantsov@gmail.com> writes:

> Usage test 'depthstencil-render-miplevels 200 s=z24_s8' causes core dump on exit.
> Issues:
> 1. Allocation of memory many times to the same variable
> 2. Not complete array to store pointers
> 3. Absence of memory freeing

Can you explain how not freeing memory could cause a core dump on exit?
And, in general, I would recommend just freeing data after usage, rather
than having a deinit function.
Hello, Eric
>
> Can you explain how not freeing memory could cause a core dump on exit?

Core mostly was generated due: a number of levels is max_miplevel (level 0
is also available, so index should be counted as <=max_miplevel), but array
that holds pointers is size of max_miplevel (so without counting of
0-level). In that way happens array index out of bounds + memory allocation
to some undefined address.


> And, in general, I would recommend just freeing data after usage,
> rather than having a deinit function.

Looks like it may complicate a logic and code of test. Test may exit with
any call to 'piglit_report_result' and it can happens on any stage of
execution. In proposed mechanism we shouldn't care when and why exit is
done, so we can be sure that all resources will be freed.

On Fri, Nov 23, 2018 at 7:35 PM Eric Anholt <eric@anholt.net> wrote:

> Sergii Romantsov <sergii.romantsov@gmail.com> writes:
>
> > Usage test 'depthstencil-render-miplevels 200 s=z24_s8' causes core dump
> on exit.
> > Issues:
> > 1. Allocation of memory many times to the same variable
> > 2. Not complete array to store pointers
> > 3. Absence of memory freeing
>
> Can you explain how not freeing memory could cause a core dump on exit?
> And, in general, I would recommend just freeing data after usage, rather
> than having a deinit function.
>