flac dev - Jul 2018 - [PATCH 1/2] Avoid double free in iconvert()

I'm looking at an issue reported by the Coverity static analyzer.
In iconvert() in src/share/utf8/iconvert.c on line 152 there is

newbuf = safe_realloc_add_2op_(utfbuf, ...);

If the request size is not valid, the function will free utfbuf and
return 0. This is followed by goto fail and utfbuf is freed for the
second time. A simply fix would be to set utfbuf to 0 if newbuf is 0.
However, this would create a leak in the case when the size is ok, but
the reallocation itself failed. Should safe_realloc_add_2op_() be
changed to use safe_realloc_() instead of realloc()? Is there any code
in flac that relies on the current behavior?

-- 
Miroslav Lichvar

When safe_realloc_add_2op_(utfbuf, ...) is called with an invalid size
and returns 0, set utfbuf to 0 to avoid second deallocation later in the
function.
---
 src/share/utf8/iconvert.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/share/utf8/iconvert.c b/src/share/utf8/iconvert.c
index 472ca876..03068ac9 100644
--- a/src/share/utf8/iconvert.c
+++ b/src/share/utf8/iconvert.c
@@ -150,8 +150,10 @@ int iconvert(const char *fromcode, const char *tocode,
       return ret;
     }
     newbuf = safe_realloc_add_2op_(utfbuf, (ob - utfbuf), /*+*/1);
-    if (!newbuf)
+    if (!newbuf) {
+      utfbuf = 0;
       goto fail;
+    }
     ob = (ob - utfbuf) + newbuf;
     *ob = '\0';
     *to = newbuf;
-- 
2.17.1

---
 include/share/alloc.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/share/alloc.h b/include/share/alloc.h
index 914de9ba..63878db0 100644
--- a/include/share/alloc.h
+++ b/include/share/alloc.h
@@ -168,7 +168,7 @@ static inline void *safe_realloc_add_2op_(void *ptr, size_t
size1, size_t size2)
 		free(ptr);
 		return 0;
 	}
-	return realloc(ptr, size2);
+	return safe_realloc_(ptr, size2);
 }
 
 static inline void *safe_realloc_add_3op_(void *ptr, size_t size1, size_t
size2, size_t size3)
-- 
2.17.1

On Wed, Jul 18, 2018 at 12:30:41PM +0200, Miroslav Lichvar
wrote:
> Should safe_realloc_add_2op_() be
> changed to use safe_realloc_() instead of realloc()? Is there any code
> in flac that relies on the current behavior?
It does indeed look like some code that (indirectly) uses the
safe_realloc_*() functions relies on the pointer not being freed. The
reallocation errors are not handled and propagated back, so the
pointers that would be freed might be dereferenced again.

Please ignore the patches I sent. The callers need to be fixed too.
This will require a careful review of a lot of code.

-- 
Miroslav Lichvar