container: fix error leak

[giuseppe]

print the current error and release it.

Closes: #1957

[packit-as-a-service]

TMT tests failed. @containers/packit-build please check.

[eriksjolund]

LGTM

Side note:

When executing the code, we might end up inside this if statement

crun/src/libcrun/error.c

Lines 129 to 133 in 91df434

	if (err == NULL || *err == NULL || **err == NULL)
	{
	// Internal error
	return;
	}

The comment

       // Internal error

is then be a bit misleading.

Reference:

crun/src/libcrun/error.c

Line 131 in 91df434

// Internal error

crun/src/libcrun/container.c

Line 1786 in 91df434

crun_error_write_warning_and_release (context->output_handler_arg, &err);

[giuseppe]

thanks!

The code was wrong, could you please look at the new version?

[eriksjolund]

LGTM
(yes, I also prefer this version )

[eriksjolund]

After writing LGTM I started to wonder if it is more robust to use a bool

diff --git a/src/libcrun/container.c b/src/libcrun/container.c
index 50b1074c..f26e7b95 100644
--- a/src/libcrun/container.c
+++ b/src/libcrun/container.c
@@ -801,6 +801,7 @@ do_hooks (runtime_spec_schema_config_schema *def, pid_t pid, const char *id, boo
 
   ret = 0;
 
+  bool error_created = false;
   for (i = 0; i < hooks_len; i++)
     {
       char **env = environ;
@@ -809,11 +810,14 @@ do_hooks (runtime_spec_schema_config_schema *def, pid_t pid, const char *id, boo
         env = hooks[i]->env;
 
       /* Release the error from the previous iteration, if any.  */
-      if (err && *err)
+      if (error_created)
         crun_error_release (err);
 
       ret = run_process_with_stdin_timeout_envp (hooks[i]->path, hooks[i]->args, cwd, hooks[i]->timeout, env,
                                                  stdin, stdin_len, out_fd, err_fd, err);
+      if (UNLIKELY (ret < 0))
+        error_created = true;
+
       if (UNLIKELY (ret != 0))
         {
           if (keep_going)

Maybe it could happen that libcrun_error_t is uninitialized when passed in?

If so then this would be undefined behaviour

crun/src/libcrun/error.c

Lines 110 to 111 in ff8f451

	free (ptr->msg);
	free (ptr);

I haven't checked if it could happen but I see that sometimes

 libcrun_error_t err;

is used instead of

 libcrun_error_t err = NULL;

git grep "libcrun_error_t err" | grep -v "= NULL;"
python/crun_python.c:  libcrun_error_t err;
python/crun_python.c:  libcrun_error_t err;
python/crun_python.c:  libcrun_error_t err;
python/crun_python.c:  libcrun_error_t err;
python/crun_python.c:  libcrun_error_t err;
python/crun_python.c:  libcrun_error_t err;
python/crun_python.c:  libcrun_error_t err;
python/crun_python.c:  libcrun_error_t err;
python/crun_python.c:  libcrun_error_t err;
python/crun_python.c:  libcrun_error_t err;
python/crun_python.c:  libcrun_error_t err;
python/crun_python.c:  libcrun_error_t err;
src/crun.c:      libcrun_error_t err;
src/libcrun/cgroup-systemd.c:  libcrun_error_t err;
src/libcrun/cloned_binary.c:	libcrun_error_t err;
src/libcrun/handlers/krun.c:  libcrun_error_t err;
tests/tests_libcrun_utils.c:  libcrun_error_t err;

[giuseppe]

yeah that is better. Would you like to open a PR? :-)

[eriksjolund]

PR: #1965