[systemd-devel] [PATCH] Fix resource leak (coverity CID 1237760)

[Lennart Poettering]

On Wed, 17.09.14 18:10, Cristian Rodríguez (crrodriguez at opensuse.org) wrote:

> ---
>  src/libsystemd/sd-bus/bus-kernel.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libsystemd/sd-bus/bus-kernel.c b/src/libsystemd/sd-bus/bus-kernel.c
> index 505f335..b3cc996 100644
> --- a/src/libsystemd/sd-bus/bus-kernel.c
> +++ b/src/libsystemd/sd-bus/bus-kernel.c
> @@ -1446,9 +1446,11 @@ int bus_kernel_create_endpoint(const char *bus_name, const char *ep_name, char *
>          }
>  
>          if (ep_path) {
> -                asprintf(ep_path, "%s/%s", dirname(path), ep_name);
> -                if (!*ep_path)
> +                int r = asprintf(ep_path, "%s/%s", dirname(path), ep_name);
> +                if (r == -1 || !*ep_path) {
> +                        safe_close(fd);
>                          return -ENOMEM;
> +                }
>          }
>  
>          return fd;

Well, please don't invoke functions in the same lines as you declare
new variables. Please keep function invocation and variable
declaration separate. It's OK to initialize new variables with simple
expressions, but please let's separate code and variable declaration.

And while we are at this code (issues predating your patch):

This shouldn't clobber parameters on failure. We should always avoid
that. Let's not start with this, not even for internal calls like this
one.

asprintf() is slow, in cases like the above, where we just concatenate
a few strings, use strjoin(), it's measurably faster.

Fixed all three now, plus an invalid memory access.

Lennart

-- 
Lennart Poettering, Red Hat