Re: [PATCH 3/3] libsemanage/tests: free memory

From: Nicolas Iooss <nicolas.iooss@m4x.org>
To: "Christian Göttsche" <cgzones@googlemail.com>
Cc: SElinux list <selinux@vger.kernel.org>
Subject: Re: [PATCH 3/3] libsemanage/tests: free memory
Date: Sat, 16 Oct 2021 22:08:23 +0200	[thread overview]
Message-ID: <CAJfZ7=mgB=8YYJ301fFKwYtG-yhvc6pQFRAbNn4Dn7sbZFq+gg@mail.gmail.com> (raw)
In-Reply-To: <20211013125358.15534-3-cgzones@googlemail.com>

On Wed, Oct 13, 2021 at 2:54 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Free all memory in test cases, reported by LeakSanitizer.
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  libsemanage/tests/test_bool.c      | 33 +++++++++++++++++++++++++++++-
>  libsemanage/tests/test_fcontext.c  | 31 +++++++++++++++++++++++++++-
>  libsemanage/tests/test_ibendport.c | 13 ++++++++++++
>  libsemanage/tests/test_iface.c     | 24 ++++++++++++++++++++++
>  libsemanage/tests/test_node.c      | 29 ++++++++++++++++++++++++++
>  libsemanage/tests/test_other.c     |  6 ++++++
>  libsemanage/tests/test_port.c      | 24 ++++++++++++++++++++++
>  libsemanage/tests/test_user.c      | 17 +++++++++++++++
>  libsemanage/tests/utilities.c      |  5 ++++-
>  libsemanage/tests/utilities.h      |  2 ++
>  10 files changed, 181 insertions(+), 3 deletions(-)

The diff was very large, so I posted 3 comments on GitHub:
https://github.com/SELinuxProject/selinux/pull/321/commits/322243ee7d34f1b3e23c04c75a6b5c4f597092f7#r730306247
and https://github.com/SELinuxProject/selinux/pull/321/commits/322243ee7d34f1b3e23c04c75a6b5c4f597092f7#r730306949
and https://github.com/SELinuxProject/selinux/pull/321/commits/322243ee7d34f1b3e23c04c75a6b5c4f597092f7#r730307266
, about the same pattern which causes (in my humble opinion) issues.
The pattern is in:

CU_ASSERT(semanage_ibendport_query_local(sh, key,
&ibendport_local) >= 0);
CU_ASSERT_PTR_NOT_NULL_FATAL(ibendport_local);
CU_ASSERT(semanage_ibendport_del_local(sh, key) >= 0);
CU_ASSERT(semanage_ibendport_query_local(sh, key,
&ibendport_local) < 0);

/* cleanup */
semanage_ibendport_key_free(key);
semanage_ibendport_free(ibendport_local);

The last ..._free occurs after a second call to
semanage_ibendport_query_local. It seems more appropriate to free the
memory right after CU_ASSERT_PTR_NOT_NULL_FATAL(ibendport_local),
before the second semanage_ibendport_query_local. So if an error
happens, there is no memory issue too.

This pattern is similar when testing boolean, fiface, node, port, user
and context objects.

What do you think?

Thanks,
Nicolas