[Toybox] [PATCH] losetup: Fix memory leaks in loopback_setup()

[Alessio Balsini]

Hi James,

You are absolutely right, that "s" pointer to dynamic memory, followed by
"free(s)" and "FLAG(s)" is confusing. This change  would be out of context wrt
my previous patch, so here follows another patch that fixes the misleading
naming.

Thank you for pointing that out.
Alessio

On Mon, Oct 21, 2019 at 09:12:27PM -0700, James McMechan wrote:
> Slight glitch for humans, the line "if (FLAG(s)) " after the free(s);
> Even though that 's' is a flag name, it looks at first and maybe second
> glance like a use after free. Either a comment explaining that or choosing
> a variable other than 's' for the allocation might be clearer.
> 
> 
> On Mon, Oct 21, 2019, 03:22 Alessio Balsini via Toybox <
> toybox at lists.landley.net> wrote:
> 
> > The function loopback_setup() uses xabspath() to get the loopback path.
> > This function allocates dynamic memory which should be freed by the
> > function caller.
> > But there are early return cases where the dynamic memory is not freed.
> > Besides the special cases of perror_exit(), for which the "early" free
> > operation is simply used to silence memory analysis tools, the
> >
> >   if (racy && errno == EBUSY) return 1;
> >
> > branch may be a real cause of memory leak.
> >
> > Fix by adding a new free() in the racy+EBUSY branch and anticipating the
> > existing free().
> >
> > Signed-off-by: Alessio Balsini <balsini at android.com>
> > ---
> >  toys/other/losetup.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/toys/other/losetup.c b/toys/other/losetup.c
> > index 917e64ea..7f91ba1f 100644
> > --- a/toys/other/losetup.c
> > +++ b/toys/other/losetup.c
> > @@ -107,15 +107,16 @@ static int loopback_setup(char *device, char *file)
> >
> >      if (!s) perror_exit("file"); // already opened, but if deleted
> > since...
> >      if (ioctl(lfd, LOOP_SET_FD, ffd)) {
> > +      free(s);
> >        if (racy && errno == EBUSY) return 1;
> >        perror_exit("%s=%s", device, file);
> >      }
> > +    xstrncpy((char *)loop->lo_file_name, s, LO_NAME_SIZE);
> > +    free(s);
> >      loop->lo_offset = TT.o;
> >      loop->lo_sizelimit = TT.S;
> > -    xstrncpy((char *)loop->lo_file_name, s, LO_NAME_SIZE);
> >      if (ioctl(lfd, LOOP_SET_STATUS64, loop)) perror_exit("%s=%s", device,
> > file);
> >      if (FLAG(s)) puts(device);
> > -    free(s);
> >    }
> >    else {
> >      xprintf("%s: [%lld]:%llu (%s)", device, (long long)loop->lo_device,
> > --
> > 2.23.0.866.gb869b98d4c-goog
> >
> > _______________________________________________
> > Toybox mailing list
> > Toybox at lists.landley.net
> > http://lists.landley.net/listinfo.cgi/toybox-landley.net
> >
> 
> -- 

-- >8 --
>From 72f3eabd9f43de775b9f3cae17e9c9303940f7ff Mon Sep 17 00:00:00 2001
From: Alessio Balsini <balsini at android.com>
Date: Tue, 22 Oct 2019 11:31:05 +0100
Subject: [PATCH] losetup: Change variable name to improve readability

Having a dynamic memory pointer named as "s", issuing "free(s)" and then
performing "FLAG(s)" is correct: "FLAG(s)" is a macro which uses "s" as
a token and expands as "FLAG_s". At a glance, this would instead look
like a use-after-free violation.
Fix this readability issue by renaming the "s" pointer variable to
"f_path".

Change-Id: I51f139034a7dcd67a08a6952bc22c1a904162c65
Signed-off-by: Alessio Balsini <balsini at android.com>
---
 toys/other/losetup.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/toys/other/losetup.c b/toys/other/losetup.c
index 7f91ba1f..27d25a18 100644
--- a/toys/other/losetup.c
+++ b/toys/other/losetup.c
@@ -103,16 +103,16 @@ static int loopback_setup(char *device, char *file)
     }
   // Associate file with this device?
   } else if (file) {
-    char *s = xabspath(file, 1);
+    char *f_path = xabspath(file, 1);
 
-    if (!s) perror_exit("file"); // already opened, but if deleted since...
+    if (!f_path) perror_exit("file"); // already opened, but if deleted since...
     if (ioctl(lfd, LOOP_SET_FD, ffd)) {
-      free(s);
+      free(f_path);
       if (racy && errno == EBUSY) return 1;
       perror_exit("%s=%s", device, file);
     }
-    xstrncpy((char *)loop->lo_file_name, s, LO_NAME_SIZE);
-    free(s);
+    xstrncpy((char *)loop->lo_file_name, f_path, LO_NAME_SIZE);
+    free(f_path);
     loop->lo_offset = TT.o;
     loop->lo_sizelimit = TT.S;
     if (ioctl(lfd, LOOP_SET_STATUS64, loop)) perror_exit("%s=%s", device, file);
-- 
2.23.0.866.gb869b98d4c-goog