On 4/30/22 14:11, Bruno Haible wrote:
This is a false positive as well:
Thanks for checking that. I did a similar check before seeing your
email, and found some opportunities for simplifying the code so that
these checks could be easier in the future. (With luck it'd also help
avoid false positives from lower-quality static checkers, which would
save us time in the future.) What do you think of the attached patch?
A bonus is that it shrinks the size of the vasnprintf text by about 7%
on Fedora 35 x86-64.From 9fcda8e08c51791e35441cc19c4d9275211b02f0 Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Sat, 30 Apr 2022 15:57:48 -0700
Subject: [PATCH] vasnprintf: simplify cleanup code
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This shrinks the text size by 7% on my platform,
and makes it a bit easier to understand (for me at least....).
* lib/vasnprintf.c (divide, VASNPRINTF):
Just call free (x) instead of doing ‘if (x != NULL) free (x);’.
(VASNPRINTF): Simplify by coalescing cleanup code.
Preserve malloc, realloc errno instead of replacing
it with ENOMEM.
(CLEANUP): Remove; no longer needed.
* modules/c-vasnprintf, modules/vasnprintf (Depends-on):
Depend on malloc-posix, realloc-posix so that we can
rely on errno being set on failure.
---
ChangeLog | 15 +++
lib/vasnprintf.c | 298 ++++++++++---------------------------------
modules/c-vasnprintf | 2 +
modules/vasnprintf | 2 +
4 files changed, 85 insertions(+), 232 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 7c7ed13141..5e7f0de809 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,18 @@
+2022-04-30 Paul Eggert <egg...@cs.ucla.edu>
+
+ vasnprintf: simplify cleanup code
+ This shrinks the text size by 7% on my platform,
+ and makes it a bit easier to understand (for me at least....).
+ * lib/vasnprintf.c (divide, VASNPRINTF):
+ Just call free (x) instead of doing ‘if (x != NULL) free (x);’.
+ (VASNPRINTF): Simplify by coalescing cleanup code.
+ Preserve malloc, realloc errno instead of replacing
+ it with ENOMEM.
+ (CLEANUP): Remove; no longer needed.
+ * modules/c-vasnprintf, modules/vasnprintf (Depends-on):
+ Depend on malloc-posix, realloc-posix so that we can
+ rely on errno being set on failure.
+
2022-04-30 Bruno Haible <br...@clisp.org>
string: Avoid syntax error on glibc systems with GCC 11.
diff --git a/lib/vasnprintf.c b/lib/vasnprintf.c
index 485745243f..d20d30dc9f 100644
--- a/lib/vasnprintf.c
+++ b/lib/vasnprintf.c
@@ -915,8 +915,7 @@ divide (mpn_t a, mpn_t b, mpn_t *q)
q_ptr[q_len++] = 1;
}
keep_q:
- if (tmp_roomptr != NULL)
- free (tmp_roomptr);
+ free (tmp_roomptr);
q->limbs = q_ptr;
q->nlimbs = q_len;
return roomptr;
@@ -1865,29 +1864,19 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
/* errno is already set. */
return NULL;
- /* Frees the memory allocated by this function. Preserves errno. */
-#define CLEANUP() \
- if (d.dir != d.direct_alloc_dir) \
- free (d.dir); \
- if (a.arg != a.direct_alloc_arg) \
- free (a.arg);
+ TCHAR_T *buf_malloced = NULL;
+ /* Output string accumulator. */
+ DCHAR_T *result = resultbuf;
if (PRINTF_FETCHARGS (args, &a) < 0)
- {
- CLEANUP ();
- errno = EINVAL;
- return NULL;
- }
-
- {
+ errno = EINVAL;
+ else
+ {
size_t buf_neededlength;
TCHAR_T *buf;
- TCHAR_T *buf_malloced;
const FCHAR_T *cp;
size_t i;
DIRECTIVE *dp;
- /* Output string accumulator. */
- DCHAR_T *result;
size_t allocated;
size_t length;
@@ -1897,32 +1886,20 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
xsum4 (7, d.max_width_length, d.max_precision_length, 6);
#if HAVE_ALLOCA
if (buf_neededlength < 4000 / sizeof (TCHAR_T))
- {
- buf = (TCHAR_T *) alloca (buf_neededlength * sizeof (TCHAR_T));
- buf_malloced = NULL;
- }
+ buf = (TCHAR_T *) alloca (buf_neededlength * sizeof (TCHAR_T));
else
#endif
{
size_t buf_memsize = xtimes (buf_neededlength, sizeof (TCHAR_T));
if (size_overflow_p (buf_memsize))
- goto out_of_memory_1;
+ goto out_of_memory;
buf = (TCHAR_T *) malloc (buf_memsize);
if (buf == NULL)
- goto out_of_memory_1;
+ goto fail;
buf_malloced = buf;
}
- if (resultbuf != NULL)
- {
- result = resultbuf;
- allocated = *lengthp;
- }
- else
- {
- result = NULL;
- allocated = 0;
- }
+ allocated = result != NULL ? *lengthp : 0;
length = 0;
/* Invariants:
result is either == resultbuf or == NULL or malloc-allocated.
@@ -1942,7 +1919,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
memory_size = xtimes (allocated, sizeof (DCHAR_T)); \
if (size_overflow_p (memory_size)) \
oom_statement \
- if (result == resultbuf || result == NULL) \
+ if (result == resultbuf) \
memory = (DCHAR_T *) malloc (memory_size); \
else \
memory = (DCHAR_T *) realloc (result, memory_size); \
@@ -2112,15 +2089,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
if (count == 0)
break;
if (count < 0)
- {
- if (!(result == resultbuf || result == NULL))
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
- errno = EILSEQ;
- return NULL;
- }
+ goto cannot_convert;
arg_end += count;
characters++;
}
@@ -2137,15 +2106,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
if (count == 0)
break;
if (count < 0)
- {
- if (!(result == resultbuf || result == NULL))
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
- errno = EILSEQ;
- return NULL;
- }
+ goto cannot_convert;
arg_end += count;
characters++;
}
@@ -2191,14 +2152,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
converted, &converted_len);
# endif
if (converted == NULL)
- {
- if (!(result == resultbuf || result == NULL))
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
- return NULL;
- }
+ goto fail;
if (converted != result + length)
{
ENSURE_ALLOCATION_ELSE (xsum (length, converted_len),
@@ -2237,15 +2191,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
if (count == 0)
break;
if (count < 0)
- {
- if (!(result == resultbuf || result == NULL))
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
- errno = EILSEQ;
- return NULL;
- }
+ goto cannot_convert;
arg_end += count;
characters++;
}
@@ -2262,15 +2208,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
if (count == 0)
break;
if (count < 0)
- {
- if (!(result == resultbuf || result == NULL))
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
- errno = EILSEQ;
- return NULL;
- }
+ goto cannot_convert;
arg_end += count;
characters++;
}
@@ -2316,14 +2254,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
converted, &converted_len);
# endif
if (converted == NULL)
- {
- if (!(result == resultbuf || result == NULL))
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
- return NULL;
- }
+ goto fail;
if (converted != result + length)
{
ENSURE_ALLOCATION_ELSE (xsum (length, converted_len),
@@ -2362,15 +2293,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
if (count == 0)
break;
if (count < 0)
- {
- if (!(result == resultbuf || result == NULL))
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
- errno = EILSEQ;
- return NULL;
- }
+ goto cannot_convert;
arg_end += count;
characters++;
}
@@ -2387,15 +2310,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
if (count == 0)
break;
if (count < 0)
- {
- if (!(result == resultbuf || result == NULL))
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
- errno = EILSEQ;
- return NULL;
- }
+ goto cannot_convert;
arg_end += count;
characters++;
}
@@ -2441,14 +2356,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
converted, &converted_len);
# endif
if (converted == NULL)
- {
- if (!(result == resultbuf || result == NULL))
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
- return NULL;
- }
+ goto fail;
if (converted != result + length)
{
ENSURE_ALLOCATION_ELSE (xsum (length, converted_len),
@@ -2590,16 +2498,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
/* Found the terminating NUL. */
break;
if (count < 0)
- {
- /* Invalid or incomplete multibyte character. */
- if (!(result == resultbuf || result == NULL))
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
- errno = EILSEQ;
- return NULL;
- }
+ goto cannot_convert;
arg_end += count;
characters++;
}
@@ -2626,16 +2525,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
/* Found the terminating NUL. */
break;
if (count < 0)
- {
- /* Invalid or incomplete multibyte character. */
- if (!(result == resultbuf || result == NULL))
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
- errno = EILSEQ;
- return NULL;
- }
+ goto cannot_convert;
arg_end += count;
characters++;
}
@@ -2752,16 +2642,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
break;
count = local_wcrtomb (cbuf, *arg_end, &state);
if (count < 0)
- {
- /* Cannot convert. */
- if (!(result == resultbuf || result == NULL))
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
- errno = EILSEQ;
- return NULL;
- }
+ goto cannot_convert;
if (precision < (unsigned int) count)
break;
arg_end++;
@@ -2793,16 +2674,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
break;
count = local_wcrtomb (cbuf, *arg_end, &state);
if (count < 0)
- {
- /* Cannot convert. */
- if (!(result == resultbuf || result == NULL))
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
- errno = EILSEQ;
- return NULL;
- }
+ goto cannot_convert;
arg_end++;
characters += count;
}
@@ -2821,7 +2693,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
/* Convert the string into a piece of temporary memory. */
tmpsrc = (TCHAR_T *) malloc (characters * sizeof (TCHAR_T));
if (tmpsrc == NULL)
- goto out_of_memory;
+ goto fail;
{
TCHAR_T *tmpptr = tmpsrc;
size_t remaining;
@@ -2859,12 +2731,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
if (tmpdst == NULL)
{
free (tmpsrc);
- if (!(result == resultbuf || result == NULL))
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
- return NULL;
+ goto fail;
}
free (tmpsrc);
# endif
@@ -2938,16 +2805,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
abort ();
count = local_wcrtomb (cbuf, *arg, &state);
if (count <= 0)
- {
- /* Cannot convert. */
- if (!(result == resultbuf || result == NULL))
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
- errno = EILSEQ;
- return NULL;
- }
+ goto cannot_convert;
ENSURE_ALLOCATION (xsum (length, count));
memcpy (result + length, cbuf, count);
length += count;
@@ -3083,14 +2941,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
NULL,
NULL, &tmpdst_len);
if (tmpdst == NULL)
- {
- if (!(result == resultbuf || result == NULL))
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
- return NULL;
- }
+ goto fail;
# endif
if (has_width)
@@ -3295,8 +3146,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
goto out_of_memory;
tmp = (DCHAR_T *) malloc (tmp_memsize);
if (tmp == NULL)
- /* Out of memory. */
- goto out_of_memory;
+ goto fail;
}
pad_ptr = NULL;
@@ -3839,8 +3689,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
goto out_of_memory;
tmp = (DCHAR_T *) malloc (tmp_memsize);
if (tmp == NULL)
- /* Out of memory. */
- goto out_of_memory;
+ goto fail;
}
pad_ptr = NULL;
@@ -3910,7 +3759,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
if (digits == NULL)
{
END_LONG_DOUBLE_ROUNDING ();
- goto out_of_memory;
+ goto fail;
}
ndigits = strlen (digits);
@@ -3970,7 +3819,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
if (digits == NULL)
{
END_LONG_DOUBLE_ROUNDING ();
- goto out_of_memory;
+ goto fail;
}
ndigits = strlen (digits);
@@ -4006,7 +3855,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
{
free (digits);
END_LONG_DOUBLE_ROUNDING ();
- goto out_of_memory;
+ goto fail;
}
if (strlen (digits2) == precision + 1)
{
@@ -4106,7 +3955,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
if (digits == NULL)
{
END_LONG_DOUBLE_ROUNDING ();
- goto out_of_memory;
+ goto fail;
}
ndigits = strlen (digits);
@@ -4142,7 +3991,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
{
free (digits);
END_LONG_DOUBLE_ROUNDING ();
- goto out_of_memory;
+ goto fail;
}
if (strlen (digits2) == precision)
{
@@ -4373,7 +4222,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
digits =
scale10_round_decimal_double (arg, precision);
if (digits == NULL)
- goto out_of_memory;
+ goto fail;
ndigits = strlen (digits);
if (ndigits > precision)
@@ -4430,7 +4279,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
scale10_round_decimal_double (arg,
(int)precision - exponent);
if (digits == NULL)
- goto out_of_memory;
+ goto fail;
ndigits = strlen (digits);
if (ndigits == precision + 1)
@@ -4464,7 +4313,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
if (digits2 == NULL)
{
free (digits);
- goto out_of_memory;
+ goto fail;
}
if (strlen (digits2) == precision + 1)
{
@@ -4578,7 +4427,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
scale10_round_decimal_double (arg,
(int)(precision - 1) - exponent);
if (digits == NULL)
- goto out_of_memory;
+ goto fail;
ndigits = strlen (digits);
if (ndigits == precision)
@@ -4612,7 +4461,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
if (digits2 == NULL)
{
free (digits);
- goto out_of_memory;
+ goto fail;
}
if (strlen (digits2) == precision)
{
@@ -5011,8 +4860,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
goto out_of_memory;
tmp = (TCHAR_T *) malloc (tmp_memsize);
if (tmp == NULL)
- /* Out of memory. */
- goto out_of_memory;
+ goto fail;
}
#endif
@@ -5463,13 +5311,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
errno = EINVAL;
}
- if (!(result == resultbuf || result == NULL))
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
-
- return NULL;
+ goto fail;
}
#if USE_SNPRINTF
@@ -5484,7 +5326,10 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
allocating more memory will not increase maxlen.
Instead of looping, bail out. */
if (maxlen == INT_MAX / TCHARS_PER_DCHAR)
- goto overflow;
+ {
+ errno = EOVERFLOW;
+ goto fail;
+ }
else
{
/* Need at least (count + 1) * sizeof (TCHAR_T)
@@ -5603,14 +5448,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
NULL,
NULL, &tmpdst_len);
if (tmpdst == NULL)
- {
- if (!(result == resultbuf || result == NULL))
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
- return NULL;
- }
+ goto fail;
ENSURE_ALLOCATION_ELSE (xsum (length, tmpdst_len),
{ free (tmpdst); goto out_of_memory; });
DCHAR_CPY (result + length, tmpdst, tmpdst_len);
@@ -5823,37 +5661,33 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
result = memory;
}
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
+ free (buf_malloced);
*lengthp = length;
/* Note that we can produce a big string of a length > INT_MAX. POSIX
says that snprintf() fails with errno = EOVERFLOW in this case, but
that's only because snprintf() returns an 'int'. This function does
not have this limitation. */
- return result;
+ goto cleanup;
-#if USE_SNPRINTF
- overflow:
- if (!(result == resultbuf || result == NULL))
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
- errno = EOVERFLOW;
- return NULL;
-#endif
+ cannot_convert:
+ errno = EILSEQ;
+ goto fail;
out_of_memory:
- if (!(result == resultbuf || result == NULL))
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- out_of_memory_1:
- CLEANUP ();
errno = ENOMEM;
- return NULL;
- }
+ }
+
+ fail:
+ if (result != resultbuf)
+ free (result);
+ free (buf_malloced);
+ result = NULL;
+ cleanup:
+ if (d.dir != d.direct_alloc_dir)
+ free (d.dir);
+ if (a.arg != a.direct_alloc_arg)
+ free (a.arg);
+ return result;
}
#undef MAX_ROOM_NEEDED
diff --git a/modules/c-vasnprintf b/modules/c-vasnprintf
index f193f70856..67bfcb29d3 100644
--- a/modules/c-vasnprintf
+++ b/modules/c-vasnprintf
@@ -32,6 +32,8 @@ printf-frexpl
signbit
fpucw
free-posix
+malloc-posix
+realloc-posix
nocrash
printf-safe
alloca-opt
diff --git a/modules/vasnprintf b/modules/vasnprintf
index bcde2b9cfd..f4babaf201 100644
--- a/modules/vasnprintf
+++ b/modules/vasnprintf
@@ -26,6 +26,8 @@ alloca-opt
attribute
float
free-posix
+malloc-posix
+realloc-posix
stdint
xsize
errno
--
2.34.1
