<div dir="auto"><div class="gmail_quote" dir="auto"><div dir="ltr" class="gmail_attr"><span style="font-family:sans-serif">Slight glitch for humans, the line "if (FLAG(s)) " after the free(s);</span></div><div dir="ltr" class="gmail_attr"><span style="font-family:sans-serif">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.</span></div><div dir="ltr" class="gmail_attr"><span style="font-family:sans-serif"><br></span></div><div dir="ltr" class="gmail_attr"><br></div><div dir="ltr" class="gmail_attr">On Mon, Oct 21, 2019, 03:22 Alessio Balsini via Toybox <<a href="mailto:toybox@lists.landley.net" target="_blank" rel="noreferrer">toybox@lists.landley.net</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">The function loopback_setup() uses xabspath() to get the loopback path.<br>
This function allocates dynamic memory which should be freed by the<br>
function caller.<br>
But there are early return cases where the dynamic memory is not freed.<br>
Besides the special cases of perror_exit(), for which the "early" free<br>
operation is simply used to silence memory analysis tools, the<br>
<br>
  if (racy && errno == EBUSY) return 1;<br>
<br>
branch may be a real cause of memory leak.<br>
<br>
Fix by adding a new free() in the racy+EBUSY branch and anticipating the<br>
existing free().<br>
<br>
Signed-off-by: Alessio Balsini <<a href="mailto:balsini@android.com" rel="noreferrer noreferrer" target="_blank">balsini@android.com</a>><br>
---<br>
 toys/other/losetup.c | 5 +++--<br>
 1 file changed, 3 insertions(+), 2 deletions(-)<br>
<br>
diff --git a/toys/other/losetup.c b/toys/other/losetup.c<br>
index 917e64ea..7f91ba1f 100644<br>
--- a/toys/other/losetup.c<br>
+++ b/toys/other/losetup.c<br>
@@ -107,15 +107,16 @@ static int loopback_setup(char *device, char *file)<br>
<br>
     if (!s) perror_exit("file"); // already opened, but if deleted since...<br>
     if (ioctl(lfd, LOOP_SET_FD, ffd)) {<br>
+      free(s);<br>
       if (racy && errno == EBUSY) return 1;<br>
       perror_exit("%s=%s", device, file);<br>
     }<br>
+    xstrncpy((char *)loop->lo_file_name, s, LO_NAME_SIZE);<br>
+    free(s);<br>
     loop->lo_offset = TT.o;<br>
     loop->lo_sizelimit = TT.S;<br>
-    xstrncpy((char *)loop->lo_file_name, s, LO_NAME_SIZE);<br>
     if (ioctl(lfd, LOOP_SET_STATUS64, loop)) perror_exit("%s=%s", device, file);<br>
     if (FLAG(s)) puts(device);<br>
-    free(s);<br>
   }<br>
   else {<br>
     xprintf("%s: [%lld]:%llu (%s)", device, (long long)loop->lo_device,<br>
-- <br>
2.23.0.866.gb869b98d4c-goog<br>
<br>
_______________________________________________<br>
Toybox mailing list<br>
<a href="mailto:Toybox@lists.landley.net" rel="noreferrer noreferrer" target="_blank">Toybox@lists.landley.net</a><br>
<a href="http://lists.landley.net/listinfo.cgi/toybox-landley.net" rel="noreferrer noreferrer noreferrer" target="_blank">http://lists.landley.net/listinfo.cgi/toybox-landley.net</a><br>
</blockquote></div></div>