From 9fcda8e08c51791e35441cc19c4d9275211b02f0 Mon Sep 17 00:00:00 2001 From: Paul Eggert 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 + + 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 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