* vasnprintf.c: "out_of_memory", -Wanalyzer-free-of-non-heap, -Wanalyzer-malloc-leak
@ 2022-04-30 14:13 Bjarni Ingi Gislason
2022-04-30 20:27 ` Paul Eggert
2022-04-30 21:11 ` Bruno Haible
0 siblings, 2 replies; 12+ messages in thread
From: Bjarni Ingi Gislason @ 2022-04-30 14:13 UTC (permalink / raw
To: bug-gnulib
With latest gnulib version:
commit d6a07b4dc21b3118727743142c678858df442853 (origin/master, origin/HEAD)
Author: Simon Marchi <simon.marchi@efficios.com>
Date: Fri Apr 29 01:47:55 2022 +0200
While compiling "groff" (for exampe while creating "grodvi") with
gcc-11.3 options "-fanalyzer -Wanalyzer-mismatching-deallocation"
warnings were issued:
In function 'vasnprintf':
../lib/vasnprintf.c:5849:7: warning: 'free' of 'result_334' which points
to memory not on the heap [CWE-590] [-Wanalyzer-free-of-non-heap]
5849 | free (result);
...
and
../lib/vasnprintf.c:5855:5: warning: leak of 'result_20' [CWE-401]
[-Wanalyzer-malloc-leak]
5855 | return NULL;
...
Major part of the output was:
AR lib/libgnu.a
CXXLD grodvi
In function 'vasnprintf':
../lib/vasnprintf.c:5849:7: warning: 'free' of 'result_334' which points to memory not on the heap [CWE-590] [-Wanalyzer-free-of-non-heap]
5849 | free (result);
| ^
'rpl_fprintf': events 1-2
|
|../lib/fprintf.c:36:1:
| 36 | fprintf (FILE *fp, const char *format, ...)
| | ^
| | |
| | (1) entry to 'rpl_fprintf'
|......
| 45 | output = vasnprintf (buf, &lenbuf, format, args);
| | ~
| | |
| | (2) calling 'vasnprintf' from 'rpl_fprintf'
|
+--> 'vasnprintf': events 3-4
|
|../lib/vasnprintf.c:1858:1:
| 1858 | VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
| | ^
| | |
| | (3) entry to 'vasnprintf'
|......
| 1864 | if (PRINTF_PARSE (format, &d, &a) < 0)
| | ~
| | |
| | (4) calling 'printf_parse' from 'vasnprintf'
|
+--> 'printf_parse': events 5-6
|
|../lib/printf-parse.c:74:1:
| 74 | PRINTF_PARSE (const CHAR_T *format, DIRECTIVES *d, arguments *a)
| | ^
| | |
| | (5) entry to 'printf_parse'
|......
| 128 | if (c == '%')
| | ~
| | |
| | (6) following 'false' branch...
|
'printf_parse': event 7
|
|lto1:
| (7): ...to here
|
<------+
|
'vasnprintf': events 8-11
|
|../lib/vasnprintf.c:1864:7:
| 1864 | if (PRINTF_PARSE (format, &d, &a) < 0)
| | ~^
| | ||
| | |(8) returning to 'vasnprintf' from 'printf_parse'
| | (9) following 'false' branch...
|......
| 1875 | if (PRINTF_FETCHARGS (args, &a) < 0)
| | ~ ~
| | | |
| | | (11) calling 'printf_fetchargs' from 'vasnprintf'
| | (10) ...to here
|
+--> 'printf_fetchargs': events 12-13
|
|../lib/printf-args.c:36:1:
| 36 | PRINTF_FETCHARGS (va_list args, arguments *a)
| | ^
| | |
| | (12) entry to 'printf_fetchargs'
|......
| 41 | for (i = 0, ap = &a->arg[0]; i < a->count; i++, ap++)
| | ~
| | |
| | (13) following 'false' branch...
|
'printf_fetchargs': event 14
|
|lto1:
| (14): ...to here
|
<------+
|
'vasnprintf': events 15-17
|
|../lib/vasnprintf.c:1875:7:
| 1875 | if (PRINTF_FETCHARGS (args, &a) < 0)
| | ~^
| | ||
| | |(15) returning to 'vasnprintf' from 'printf_fetchargs'
| | (16) following 'false' branch...
|......
| 1883 | size_t buf_neededlength;
| | ~
| | |
| | (17) ...to here
|
'vasnprintf': events 18-19
|
|../lib/xsize.h:66:30:
| 66 | return (sum >= size1 ? sum : SIZE_MAX);
| | ^
| | |
| | (18) following 'true' branch...
|......
| 80 | return xsum (xsum (xsum (size1, size2), size3), size4);
| | ~
| | |
| | (19) ...to here
|
'vasnprintf': events 20-30
|
|../lib/vasnprintf.c:1899:8:
| 1899 | if (buf_neededlength < 4000 / sizeof (TCHAR_T))
| | ^
| | |
| | (20) following 'true' branch (when 'sum_708 <= 3999')...
| 1900 | {
| 1901 | buf = (TCHAR_T *) alloca (buf_neededlength * sizeof (TCHAR_T));
| | ~
| | |
| | (21) ...to here
|......
| 1916 | if (resultbuf != NULL)
| | ~
| | |
| | (22) following 'true' branch (when 'resultbuf_441(D)' is non-NULL)...
| 1917 | {
| 1918 | result = resultbuf;
| | ~
| | |
| | (23) ...to here
|......
| 1960 | if (cp != dp->dir_start)
| | ~
| | |
| | (24) following 'true' branch...
| 1961 | {
| 1962 | size_t n = dp->dir_start - cp;
| | ~
| | |
| | (25) ...to here
|......
| 1965 | ENSURE_ALLOCATION (augmented_length);
| | ~
| | |
| | (26) following 'true' branch...
| | (27) ...to here
|......
| 5848 | if (!(result == resultbuf || result == NULL))
| | ~
| | |
| | (28) following 'true' branch...
| 5849 | free (result);
| | ~
| | |
| | (29) ...to here
| | (30) call to 'free' here
|
../lib/vasnprintf.c:5855:5: warning: leak of 'result_20' [CWE-401] [-Wanalyzer-malloc-leak]
5855 | return NULL;
| ^
'vasnprintf': events 1-2
|
| 1858 | VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
| | ^
| | |
| | (1) entry to 'vasnprintf'
|......
| 1864 | if (PRINTF_PARSE (format, &d, &a) < 0)
| | ~
| | |
| | (2) calling 'printf_parse' from 'vasnprintf'
|
+--> 'printf_parse': events 3-4
|
|../lib/printf-parse.c:74:1:
| 74 | PRINTF_PARSE (const CHAR_T *format, DIRECTIVES *d, arguments *a)
| | ^
| | |
| | (3) entry to 'printf_parse'
|......
| 128 | if (c == '%')
| | ~
| | |
| | (4) following 'false' branch...
|
'printf_parse': event 5
|
|lto1:
| (5): ...to here
|
<------+
|
'vasnprintf': events 6-9
|
|../lib/vasnprintf.c:1864:7:
| 1864 | if (PRINTF_PARSE (format, &d, &a) < 0)
| | ~^
| | ||
| | |(6) returning to 'vasnprintf' from 'printf_parse'
| | (7) following 'false' branch...
|......
| 1875 | if (PRINTF_FETCHARGS (args, &a) < 0)
| | ~ ~
| | | |
| | | (9) calling 'printf_fetchargs' from 'vasnprintf'
| | (8) ...to here
|
+--> 'printf_fetchargs': events 10-11
|
|../lib/printf-args.c:36:1:
| 36 | PRINTF_FETCHARGS (va_list args, arguments *a)
| | ^
| | |
| | (10) entry to 'printf_fetchargs'
|......
| 41 | for (i = 0, ap = &a->arg[0]; i < a->count; i++, ap++)
| | ~
| | |
| | (11) following 'false' branch...
|
'printf_fetchargs': event 12
|
|lto1:
| (12): ...to here
|
<------+
|
'vasnprintf': events 13-15
|
|../lib/vasnprintf.c:1875:7:
| 1875 | if (PRINTF_FETCHARGS (args, &a) < 0)
| | ~^
| | ||
| | |(13) returning to 'vasnprintf' from 'printf_fetchargs'
| | (14) following 'false' branch...
|......
| 1883 | size_t buf_neededlength;
| | ~
| | |
| | (15) ...to here
|
'vasnprintf': events 16-17
|
|../lib/xsize.h:66:30:
| 66 | return (sum >= size1 ? sum : SIZE_MAX);
| | ^
| | |
| | (16) following 'true' branch...
|......
| 80 | return xsum (xsum (xsum (size1, size2), size3), size4);
| | ~
| | |
| | (17) ...to here
|
'vasnprintf': events 18-23
|
|../lib/vasnprintf.c:1899:8:
| 1899 | if (buf_neededlength < 4000 / sizeof (TCHAR_T))
| | ^
| | |
| | (18) following 'true' branch (when 'sum_708 <= 3999')...
| 1900 | {
| 1901 | buf = (TCHAR_T *) alloca (buf_neededlength * sizeof (TCHAR_T));
| | ~
| | |
| | (19) ...to here
|......
| 1916 | if (resultbuf != NULL)
| | ~
| | |
| | (20) following 'true' branch (when 'resultbuf_441(D)' is non-NULL)...
| 1917 | {
| 1918 | result = resultbuf;
| | ~
| | |
| | (21) ...to here
|......
| 1960 | if (cp != dp->dir_start)
| | ~
| | |
| | (22) following 'true' branch...
| 1961 | {
| 1962 | size_t n = dp->dir_start - cp;
| | ~
| | |
| | (23) ...to here
|
'vasnprintf': event 24
|
|../lib/xsize.h:66:30:
| 66 | return (sum >= size1 ? sum : SIZE_MAX);
| | ^
| | |
| | (24) following 'true' branch (when 'length_348 <= sum_710')...
|
'vasnprintf': events 25-46
|
|../lib/vasnprintf.c:1963:39:
| 1963 | size_t augmented_length = xsum (length, n);
| | ^
| | |
| | (25) ...to here
| 1964 |
| 1965 | ENSURE_ALLOCATION (augmented_length);
| | ~
| | |
| | (26) following 'true' branch...
| | (27) ...to here
| | (28) following 'false' branch...
| | (29) ...to here
| | (30) following 'true' branch...
| | (31) ...to here
| | (32) allocated here
| | (33) assuming 'memory_351' is non-NULL
| | (34) following 'false' branch...
| | (35) ...to here
|......
| 1981 | if (i == d.count)
| | ~
| | |
| | (36) following 'true' branch...
|......
| 5813 | ENSURE_ALLOCATION (xsum (length, 1));
| | ~
| | |
| | (37) ...to here
| | (38) following 'true' branch...
| | (39) ...to here
|......
| 5848 | if (!(result == resultbuf || result == NULL))
| | ~
| | |
| | (40) following 'false' branch...
| 5849 | free (result);
| 5850 | if (buf_malloced != NULL)
| | ~ ~
| | | |
| | | (42) following 'false' branch (when 'buf_malloced_79' is NULL)...
| | (41) ...to here
| 5851 | free (buf_malloced);
| 5852 | out_of_memory_1:
| | ~
| | |
| | (43) ...to here
| 5853 | CLEANUP ();
| | ~ ~
| | | |
| | | (45) ...to here
| | (44) following 'false' branch...
| 5854 | errno = ENOMEM;
| 5855 | return NULL;
| | ~
| | |
| | (46) 'result_20' leaks here; was allocated at (32)
|
--
Bjarni I. Gislason
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: vasnprintf.c: "out_of_memory", -Wanalyzer-free-of-non-heap, -Wanalyzer-malloc-leak
2022-04-30 14:13 vasnprintf.c: "out_of_memory", -Wanalyzer-free-of-non-heap, -Wanalyzer-malloc-leak Bjarni Ingi Gislason
@ 2022-04-30 20:27 ` Paul Eggert
2022-05-01 17:16 ` Bjarni Ingi Gislason
2022-04-30 21:11 ` Bruno Haible
1 sibling, 1 reply; 12+ messages in thread
From: Paul Eggert @ 2022-04-30 20:27 UTC (permalink / raw
To: Bjarni Ingi Gislason; +Cc: bug-gnulib
On 4/30/22 07:13, Bjarni Ingi Gislason wrote:
> With latest gnulib version:
I'm not seeing this problem with the current
(84863a1c4dc8cca8fb0f6f670f67779cdd2d543b) gnulib version on Fedora 35
x86-64, which has GCC 11.3.1 20220421 (Red Hat 11.3.1-2).
Here's how I tried to reproduce the issue:
./gnulib-tool -h --create-testdir --dir foo vasnprintf
cd foo
./configure
make CFLAGS='-fanalyzer -Wanalyzer-mismatching-deallocation -O2' check
Does the above work for you? If so, how does it differ from what groff does?
The idea is to make the problem reproducible without dealing with groff
or with whatever changes you made to groff. If that's not possible, I
guess we'll need a copy of your groff source since it sounds like you've
modified groff.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: vasnprintf.c: "out_of_memory", -Wanalyzer-free-of-non-heap, -Wanalyzer-malloc-leak
2022-04-30 14:13 vasnprintf.c: "out_of_memory", -Wanalyzer-free-of-non-heap, -Wanalyzer-malloc-leak Bjarni Ingi Gislason
2022-04-30 20:27 ` Paul Eggert
@ 2022-04-30 21:11 ` Bruno Haible
2022-04-30 23:02 ` Paul Eggert
1 sibling, 1 reply; 12+ messages in thread
From: Bruno Haible @ 2022-04-30 21:11 UTC (permalink / raw
To: bug-gnulib; +Cc: Bjarni Ingi Gislason
Bjarni Ingi Gislason wrote:
>
> In function 'vasnprintf':
> ../lib/vasnprintf.c:5849:7: warning: 'free' of 'result_334' which points
> to memory not on the heap [CWE-590] [-Wanalyzer-free-of-non-heap]
> 5849 | free (result);
> ...
This is a false positive. By code inspection, one can see that
* the value of 'resultbuf' is never changed (in other words, this parameter
could be marked 'const'),
* staring with line 1916, the value of result is either == NULL or
== resultbuf or memory allocated within the vasnprintf function. See the
comment at line 1928.
Therefore it is safe to do
if (!(result == resultbuf || result == NULL))
free (result);
> and
>
> ../lib/vasnprintf.c:5855:5: warning: leak of 'result_20' [CWE-401]
> [-Wanalyzer-malloc-leak]
> 5855 | return NULL;
> ...
This is a false positive as well:
As mentioned above, of the control flow passes through lines 5848..5849,
'result' has been freed if it was memory allocated. In the other case,
a 'goto out_of_memory_1;' was executed; in this case 'result' did not
have any value.
Bruno
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: vasnprintf.c: "out_of_memory", -Wanalyzer-free-of-non-heap, -Wanalyzer-malloc-leak
2022-04-30 21:11 ` Bruno Haible
@ 2022-04-30 23:02 ` Paul Eggert
2022-05-01 0:24 ` Bruno Haible
0 siblings, 1 reply; 12+ messages in thread
From: Paul Eggert @ 2022-04-30 23:02 UTC (permalink / raw
To: Bruno Haible; +Cc: bug-gnulib, Bjarni Ingi Gislason
[-- Attachment #1: Type: text/plain, Size: 518 bytes --]
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.
[-- Attachment #2: 0001-vasnprintf-simplify-cleanup-code.patch --]
[-- Type: text/x-patch, Size: 28802 bytes --]
From 9fcda8e08c51791e35441cc19c4d9275211b02f0 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@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 <eggert@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 <bruno@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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: vasnprintf.c: "out_of_memory", -Wanalyzer-free-of-non-heap, -Wanalyzer-malloc-leak
2022-04-30 23:02 ` Paul Eggert
@ 2022-05-01 0:24 ` Bruno Haible
2022-05-01 2:47 ` Paul Eggert
2022-05-01 21:28 ` vasnprintf.c: "out_of_memory", -Wanalyzer-free-of-non-heap, -Wanalyzer-malloc-leak Bruno Haible
0 siblings, 2 replies; 12+ messages in thread
From: Bruno Haible @ 2022-05-01 0:24 UTC (permalink / raw
To: Paul Eggert; +Cc: bug-gnulib, Bjarni Ingi Gislason
Hi Paul,
> I did a similar check before seeing your email
It would be worth to eliminate the false positive reports by GCC. If only
we could state the invariants in a form that GCC understands. We could
use
assume (result==resultbuf)
for one part. But how to tell GCC that 'result' is heap-allocated? Is
there a __builtin_assert_malloced(p) somewhere?
> 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.
Indeed, now that we assume 'free-posix', the cleanup codes can be
simplified by setting errno first and then jumping to to common part of
the cleanup code. I like that.
I like the simplification from
if (result == resultbuf || result == NULL)
to
if (result == resultbuf)
but it needs a comment.
I don't like separating the initializations of 'result' and 'allocated',
since these two variables are strongly related.
I don't like initializing the output variables before PRINTF_FETCHARGS
has completed; that complicates the logic.
I don't like the removal of comments such as
/* Invalid or incomplete multibyte character. */
/* Cannot convert. */
Such comments help understanding the code.
> Just call free (x) instead of doing ‘if (x != NULL) free (x);’.
To me, that's OK if the probability of x being NULL is small. For
example, in 'divide', the probability is 1/32, therefore it's OK.
But buf_malloced is NULL 99% of the time; here I prefer the code
that saves a function call.
> Depend on malloc-posix, realloc-posix
These dependencies save an 'errno = ENOMEM;' assignment in one or
two places, but can cause integration problems; I am especially
thinking at the use in GNU libintl and libasprintf. Therefore here
I'll probably prefer to keep the extra assignment.
I'll rework your patch; thanks for the ideas!
Bruno
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: vasnprintf.c: "out_of_memory", -Wanalyzer-free-of-non-heap, -Wanalyzer-malloc-leak
2022-05-01 0:24 ` Bruno Haible
@ 2022-05-01 2:47 ` Paul Eggert
2022-05-01 20:28 ` malloc failing with EAGAIN Bruno Haible
2022-05-01 21:28 ` vasnprintf.c: "out_of_memory", -Wanalyzer-free-of-non-heap, -Wanalyzer-malloc-leak Bruno Haible
1 sibling, 1 reply; 12+ messages in thread
From: Paul Eggert @ 2022-05-01 2:47 UTC (permalink / raw
To: Bruno Haible; +Cc: bug-gnulib, Bjarni Ingi Gislason
On 4/30/22 17:24, Bruno Haible wrote:
> These dependencies save an 'errno = ENOMEM;' assignment in one or
> two places, but can cause integration problems; I am especially
> thinking at the use in GNU libintl and libasprintf.
Ah, I didn't know about the integration problems. I was worried about
the case where malloc fails with some errno value other than ENOMEM and
that errno value should be reflected to the user; but that's less
important than getting integration right. (malloc can fail with EAGAIN
on GNU/Linux and I assume other errno values are also possible.)
> It would be worth to eliminate the false positive reports by GCC.
Yes, though the weird thing is I'm also using GCC 11.3.1 and am not
getting the false positives.
> We could
> use
> assume (result==resultbuf)
> for one part.
I am surprised GCC doesn't deduce that itself; it's part of that same
weird thing.
If this happens only with unusual configuration settings I expect we
don't need to worry about it. It's just a warning....
> buf_malloced is NULL 99% of the time; here I prefer the code
> that saves a function call.
Good point; hadn't noticed that. I suppose this can also help make
branch prediction more accurate.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: vasnprintf.c: "out_of_memory", -Wanalyzer-free-of-non-heap, -Wanalyzer-malloc-leak
2022-04-30 20:27 ` Paul Eggert
@ 2022-05-01 17:16 ` Bjarni Ingi Gislason
2022-05-01 18:48 ` Paul Eggert
0 siblings, 1 reply; 12+ messages in thread
From: Bjarni Ingi Gislason @ 2022-05-01 17:16 UTC (permalink / raw
To: Paul Eggert; +Cc: bug-gnulib
On Sat, Apr 30, 2022 at 01:27:28PM -0700, Paul Eggert wrote:
> On 4/30/22 07:13, Bjarni Ingi Gislason wrote:
> > With latest gnulib version:
>
> I'm not seeing this problem with the current
> (84863a1c4dc8cca8fb0f6f670f67779cdd2d543b) gnulib version on Fedora 35
> x86-64, which has GCC 11.3.1 20220421 (Red Hat 11.3.1-2).
>
> Here's how I tried to reproduce the issue:
>
> ./gnulib-tool -h --create-testdir --dir foo vasnprintf
> cd foo
> ./configure
> make CFLAGS='-fanalyzer -Wanalyzer-mismatching-deallocation -O2' check
>
> Does the above work for you? If so, how does it differ from what groff does?
>
> The idea is to make the problem reproducible without dealing with groff or
> with whatever changes you made to groff. If that's not possible, I guess
> we'll need a copy of your groff source since it sounds like you've modified
> groff.
Thanks for the method to test "vasnprintf.c" separately.
I checked what of the about 30 options, I use for compiling "groff",
could cause the only warning (leak) about "vasnprintf.c" and it was the
"-flto".
The compilation of "groff" then got finished with a lot of warnings
(149) about its code.
--
Bjarni I. Gislason
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: vasnprintf.c: "out_of_memory", -Wanalyzer-free-of-non-heap, -Wanalyzer-malloc-leak
2022-05-01 17:16 ` Bjarni Ingi Gislason
@ 2022-05-01 18:48 ` Paul Eggert
0 siblings, 0 replies; 12+ messages in thread
From: Paul Eggert @ 2022-05-01 18:48 UTC (permalink / raw
To: Bjarni Ingi Gislason; +Cc: bug-gnulib
On 5/1/22 10:16, Bjarni Ingi Gislason wrote:
> I checked what of the about 30 options, I use for compiling "groff",
> could cause the only warning (leak) about "vasnprintf.c" and it was the
> "-flto".
Ah, the "-flto" suggests that the problem isn't in vasnprintf per se;
it's in groff, which isn't freeing storage allocated by vasnprintf.
Possibly it's a real memory leak, but possibly it's not really a problem
at all. It's OK - indeed, a win - to not free storage if you're about to
exit anyway.
> The compilation of "groff" then got finished with a lot of warnings
> (149) about its code.
It's up to you as to whether to investigate these warnings more
carefully. Please check to see whether they actually make sense before
reporting, due to the high number of false positives in this area.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: malloc failing with EAGAIN
2022-05-01 2:47 ` Paul Eggert
@ 2022-05-01 20:28 ` Bruno Haible
2022-05-01 22:17 ` Paul Eggert
0 siblings, 1 reply; 12+ messages in thread
From: Bruno Haible @ 2022-05-01 20:28 UTC (permalink / raw
To: Paul Eggert; +Cc: bug-gnulib, Bjarni Ingi Gislason
Paul Eggert wrote:
> malloc can fail with EAGAIN
> on GNU/Linux and I assume other errno values are also possible.
Indeed [1], although the glibc manual [2] and the Linux man page [3]
don't mention it.
The meaning of EAGAIN in this case is as documented for the mmap()
system call [4]:
"too much memory has been locked"
The usual meaning of EAGAIN [5] does not apply here:
- Stating in an error message that "Resource temporarily unavailable"
is bad, because the error does not come from outside the process, it
originates in the process itself.
- Similarly, trying again later (e.g. in a loop) will not help, since
the situation originates in the process itself.
So, IMO, applications should
- better print "Out of memory" or "Cannot allocate memory", rather than
"Resource temporarily unavailable".
- just fail, never retry.
I would argue that glibc should use a different errno value in this case.
What do you think?
Bruno
[1] https://stackoverflow.com/questions/34165276/malloc-setting-errno-to-eagain
[2] https://www.gnu.org/software/libc/manual/html_mono/libc.html
[3] https://www.kernel.org/doc/man-pages/online/pages/man3/malloc.3.html
[4] https://www.kernel.org/doc/man-pages/online/pages/man2/mmap.2.html
[5] https://www.gnu.org/software/libc/manual/html_node/Error-Codes.html
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: vasnprintf.c: "out_of_memory", -Wanalyzer-free-of-non-heap, -Wanalyzer-malloc-leak
2022-05-01 0:24 ` Bruno Haible
2022-05-01 2:47 ` Paul Eggert
@ 2022-05-01 21:28 ` Bruno Haible
2022-05-01 22:54 ` Paul Eggert
1 sibling, 1 reply; 12+ messages in thread
From: Bruno Haible @ 2022-05-01 21:28 UTC (permalink / raw
To: Paul Eggert; +Cc: bug-gnulib, Bjarni Ingi Gislason
[-- Attachment #1: Type: text/plain, Size: 150 bytes --]
Hi Paul,
I wrote:
> I'll rework your patch; thanks for the ideas!
I pushed these three patches in your name. (I hope that's fine with you.)
Bruno
[-- Attachment #2: 0001-vasnprintf-Simplify-a-free-call.patch --]
[-- Type: text/x-patch, Size: 1345 bytes --]
From da95c0389f5e8c668d6c8c16462669135f02fb38 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 1 May 2022 22:38:50 +0200
Subject: [PATCH 1/3] vasnprintf: Simplify a free() call.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
* lib/vasnprintf.c (divide): Just call
free (x) instead of doing ‘if (x != NULL) free (x);’.
---
ChangeLog | 6 ++++++
lib/vasnprintf.c | 3 +--
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 7c7ed13141..808b3756e0 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2022-05-01 Paul Eggert <eggert@cs.ucla.edu>
+
+ vasnprintf: Simplify a free() call.
+ * lib/vasnprintf.c (divide): Just call
+ free (x) instead of doing ‘if (x != NULL) free (x);’.
+
2022-04-30 Bruno Haible <bruno@clisp.org>
string: Avoid syntax error on glibc systems with GCC 11.
diff --git a/lib/vasnprintf.c b/lib/vasnprintf.c
index 485745243f..23933806df 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;
--
2.25.1
[-- Attachment #3: 0002-vasnprintf-Simplify-result-variable.patch --]
[-- Type: text/x-patch, Size: 11873 bytes --]
From eb4e77666b2c540f77328156d4c43681aa1f99b0 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 1 May 2022 22:48:07 +0200
Subject: [PATCH 2/3] vasnprintf: Simplify 'result' variable.
* lib/vasnprintf.c (VASNPRINTF): Simplify initialization and test of
'result' variable.
---
ChangeLog | 4 ++++
lib/vasnprintf.c | 57 +++++++++++++++++++++---------------------------
2 files changed, 29 insertions(+), 32 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 808b3756e0..8f6d862263 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,9 @@
2022-05-01 Paul Eggert <eggert@cs.ucla.edu>
+ vasnprintf: Simplify 'result' variable.
+ * lib/vasnprintf.c (VASNPRINTF): Simplify initialization and test of
+ 'result' variable.
+
vasnprintf: Simplify a free() call.
* lib/vasnprintf.c (divide): Just call
free (x) instead of doing ‘if (x != NULL) free (x);’.
diff --git a/lib/vasnprintf.c b/lib/vasnprintf.c
index 23933806df..da9abe4b74 100644
--- a/lib/vasnprintf.c
+++ b/lib/vasnprintf.c
@@ -1912,19 +1912,12 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
buf_malloced = buf;
}
- if (resultbuf != NULL)
- {
- result = resultbuf;
- allocated = *lengthp;
- }
- else
- {
- result = NULL;
- allocated = 0;
- }
+ result = resultbuf;
+ allocated = (resultbuf != NULL ? *lengthp : 0);
length = 0;
/* Invariants:
- result is either == resultbuf or == NULL or malloc-allocated.
+ result is either == resultbuf or malloc-allocated.
+ If result == NULL, resultbuf is == NULL as well.
If length > 0, then result != NULL. */
/* Ensures that allocated >= needed. Aborts through a jump to
@@ -1941,7 +1934,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,7 +2105,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
break;
if (count < 0)
{
- if (!(result == resultbuf || result == NULL))
+ if (result != resultbuf)
free (result);
if (buf_malloced != NULL)
free (buf_malloced);
@@ -2137,7 +2130,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
break;
if (count < 0)
{
- if (!(result == resultbuf || result == NULL))
+ if (result != resultbuf)
free (result);
if (buf_malloced != NULL)
free (buf_malloced);
@@ -2191,7 +2184,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
# endif
if (converted == NULL)
{
- if (!(result == resultbuf || result == NULL))
+ if (result != resultbuf)
free (result);
if (buf_malloced != NULL)
free (buf_malloced);
@@ -2237,7 +2230,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
break;
if (count < 0)
{
- if (!(result == resultbuf || result == NULL))
+ if (result != resultbuf)
free (result);
if (buf_malloced != NULL)
free (buf_malloced);
@@ -2262,7 +2255,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
break;
if (count < 0)
{
- if (!(result == resultbuf || result == NULL))
+ if (result != resultbuf)
free (result);
if (buf_malloced != NULL)
free (buf_malloced);
@@ -2316,7 +2309,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
# endif
if (converted == NULL)
{
- if (!(result == resultbuf || result == NULL))
+ if (result != resultbuf)
free (result);
if (buf_malloced != NULL)
free (buf_malloced);
@@ -2362,7 +2355,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
break;
if (count < 0)
{
- if (!(result == resultbuf || result == NULL))
+ if (result != resultbuf)
free (result);
if (buf_malloced != NULL)
free (buf_malloced);
@@ -2387,7 +2380,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
break;
if (count < 0)
{
- if (!(result == resultbuf || result == NULL))
+ if (result != resultbuf)
free (result);
if (buf_malloced != NULL)
free (buf_malloced);
@@ -2441,7 +2434,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
# endif
if (converted == NULL)
{
- if (!(result == resultbuf || result == NULL))
+ if (result != resultbuf)
free (result);
if (buf_malloced != NULL)
free (buf_malloced);
@@ -2591,7 +2584,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
if (count < 0)
{
/* Invalid or incomplete multibyte character. */
- if (!(result == resultbuf || result == NULL))
+ if (result != resultbuf)
free (result);
if (buf_malloced != NULL)
free (buf_malloced);
@@ -2627,7 +2620,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
if (count < 0)
{
/* Invalid or incomplete multibyte character. */
- if (!(result == resultbuf || result == NULL))
+ if (result != resultbuf)
free (result);
if (buf_malloced != NULL)
free (buf_malloced);
@@ -2753,7 +2746,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
if (count < 0)
{
/* Cannot convert. */
- if (!(result == resultbuf || result == NULL))
+ if (result != resultbuf)
free (result);
if (buf_malloced != NULL)
free (buf_malloced);
@@ -2794,7 +2787,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
if (count < 0)
{
/* Cannot convert. */
- if (!(result == resultbuf || result == NULL))
+ if (result != resultbuf)
free (result);
if (buf_malloced != NULL)
free (buf_malloced);
@@ -2858,7 +2851,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
if (tmpdst == NULL)
{
free (tmpsrc);
- if (!(result == resultbuf || result == NULL))
+ if (result != resultbuf)
free (result);
if (buf_malloced != NULL)
free (buf_malloced);
@@ -2939,7 +2932,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
if (count <= 0)
{
/* Cannot convert. */
- if (!(result == resultbuf || result == NULL))
+ if (result != resultbuf)
free (result);
if (buf_malloced != NULL)
free (buf_malloced);
@@ -3083,7 +3076,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
NULL, &tmpdst_len);
if (tmpdst == NULL)
{
- if (!(result == resultbuf || result == NULL))
+ if (result != resultbuf)
free (result);
if (buf_malloced != NULL)
free (buf_malloced);
@@ -5462,7 +5455,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
errno = EINVAL;
}
- if (!(result == resultbuf || result == NULL))
+ if (result != resultbuf)
free (result);
if (buf_malloced != NULL)
free (buf_malloced);
@@ -5603,7 +5596,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
NULL, &tmpdst_len);
if (tmpdst == NULL)
{
- if (!(result == resultbuf || result == NULL))
+ if (result != resultbuf)
free (result);
if (buf_malloced != NULL)
free (buf_malloced);
@@ -5834,7 +5827,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
#if USE_SNPRINTF
overflow:
- if (!(result == resultbuf || result == NULL))
+ if (result != resultbuf)
free (result);
if (buf_malloced != NULL)
free (buf_malloced);
@@ -5844,7 +5837,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
#endif
out_of_memory:
- if (!(result == resultbuf || result == NULL))
+ if (result != resultbuf)
free (result);
if (buf_malloced != NULL)
free (buf_malloced);
--
2.25.1
[-- Attachment #4: 0003-vasnprintf-Simplify.-Reduce-binary-code-size.patch --]
[-- Type: text/x-patch, Size: 16899 bytes --]
From bd11400942d63de12371988dca8144925de9e2c3 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 1 May 2022 23:24:02 +0200
Subject: [PATCH 3/3] vasnprintf: Simplify. Reduce binary code size.
* lib/vasnprintf.c (VASNPRINTF): Coalesce cleanup code.
---
ChangeLog | 3 +
lib/vasnprintf.c | 217 ++++++++++-------------------------------------
2 files changed, 50 insertions(+), 170 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 8f6d862263..5749e2dc69 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,8 @@
2022-05-01 Paul Eggert <eggert@cs.ucla.edu>
+ vasnprintf: Simplify. Reduce binary code size.
+ * lib/vasnprintf.c (VASNPRINTF): Coalesce cleanup code.
+
vasnprintf: Simplify 'result' variable.
* lib/vasnprintf.c (VASNPRINTF): Simplify initialization and test of
'result' variable.
diff --git a/lib/vasnprintf.c b/lib/vasnprintf.c
index da9abe4b74..285c674b9c 100644
--- a/lib/vasnprintf.c
+++ b/lib/vasnprintf.c
@@ -1872,11 +1872,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
free (a.arg);
if (PRINTF_FETCHARGS (args, &a) < 0)
- {
- CLEANUP ();
- errno = EINVAL;
- return NULL;
- }
+ goto fail_1_with_EINVAL;
{
size_t buf_neededlength;
@@ -2104,15 +2100,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
if (count == 0)
break;
if (count < 0)
- {
- if (result != resultbuf)
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
- errno = EILSEQ;
- return NULL;
- }
+ goto fail_with_EILSEQ;
arg_end += count;
characters++;
}
@@ -2129,15 +2117,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
if (count == 0)
break;
if (count < 0)
- {
- if (result != resultbuf)
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
- errno = EILSEQ;
- return NULL;
- }
+ goto fail_with_EILSEQ;
arg_end += count;
characters++;
}
@@ -2183,14 +2163,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
converted, &converted_len);
# endif
if (converted == NULL)
- {
- if (result != resultbuf)
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
- return NULL;
- }
+ goto fail_with_errno;
if (converted != result + length)
{
ENSURE_ALLOCATION_ELSE (xsum (length, converted_len),
@@ -2229,15 +2202,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
if (count == 0)
break;
if (count < 0)
- {
- if (result != resultbuf)
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
- errno = EILSEQ;
- return NULL;
- }
+ goto fail_with_EILSEQ;
arg_end += count;
characters++;
}
@@ -2254,15 +2219,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
if (count == 0)
break;
if (count < 0)
- {
- if (result != resultbuf)
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
- errno = EILSEQ;
- return NULL;
- }
+ goto fail_with_EILSEQ;
arg_end += count;
characters++;
}
@@ -2308,14 +2265,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
converted, &converted_len);
# endif
if (converted == NULL)
- {
- if (result != resultbuf)
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
- return NULL;
- }
+ goto fail_with_errno;
if (converted != result + length)
{
ENSURE_ALLOCATION_ELSE (xsum (length, converted_len),
@@ -2354,15 +2304,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
if (count == 0)
break;
if (count < 0)
- {
- if (result != resultbuf)
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
- errno = EILSEQ;
- return NULL;
- }
+ goto fail_with_EILSEQ;
arg_end += count;
characters++;
}
@@ -2379,15 +2321,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
if (count == 0)
break;
if (count < 0)
- {
- if (result != resultbuf)
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
- errno = EILSEQ;
- return NULL;
- }
+ goto fail_with_EILSEQ;
arg_end += count;
characters++;
}
@@ -2433,14 +2367,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
converted, &converted_len);
# endif
if (converted == NULL)
- {
- if (result != resultbuf)
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
- return NULL;
- }
+ goto fail_with_errno;
if (converted != result + length)
{
ENSURE_ALLOCATION_ELSE (xsum (length, converted_len),
@@ -2582,16 +2509,8 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
/* Found the terminating NUL. */
break;
if (count < 0)
- {
- /* Invalid or incomplete multibyte character. */
- if (result != resultbuf)
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
- errno = EILSEQ;
- return NULL;
- }
+ /* Invalid or incomplete multibyte character. */
+ goto fail_with_EILSEQ;
arg_end += count;
characters++;
}
@@ -2618,16 +2537,8 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
/* Found the terminating NUL. */
break;
if (count < 0)
- {
- /* Invalid or incomplete multibyte character. */
- if (result != resultbuf)
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
- errno = EILSEQ;
- return NULL;
- }
+ /* Invalid or incomplete multibyte character. */
+ goto fail_with_EILSEQ;
arg_end += count;
characters++;
}
@@ -2744,16 +2655,8 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
break;
count = local_wcrtomb (cbuf, *arg_end, &state);
if (count < 0)
- {
- /* Cannot convert. */
- if (result != resultbuf)
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
- errno = EILSEQ;
- return NULL;
- }
+ /* Cannot convert. */
+ goto fail_with_EILSEQ;
if (precision < (unsigned int) count)
break;
arg_end++;
@@ -2785,16 +2688,8 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
break;
count = local_wcrtomb (cbuf, *arg_end, &state);
if (count < 0)
- {
- /* Cannot convert. */
- if (result != resultbuf)
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
- errno = EILSEQ;
- return NULL;
- }
+ /* Cannot convert. */
+ goto fail_with_EILSEQ;
arg_end++;
characters += count;
}
@@ -2851,12 +2746,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
if (tmpdst == NULL)
{
free (tmpsrc);
- if (result != resultbuf)
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
- return NULL;
+ goto fail_with_errno;
}
free (tmpsrc);
# endif
@@ -2930,16 +2820,8 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
abort ();
count = local_wcrtomb (cbuf, *arg, &state);
if (count <= 0)
- {
- /* Cannot convert. */
- if (result != resultbuf)
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
- errno = EILSEQ;
- return NULL;
- }
+ /* Cannot convert. */
+ goto fail_with_EILSEQ;
ENSURE_ALLOCATION (xsum (length, count));
memcpy (result + length, cbuf, count);
length += count;
@@ -3075,14 +2957,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
NULL,
NULL, &tmpdst_len);
if (tmpdst == NULL)
- {
- if (result != resultbuf)
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
- return NULL;
- }
+ goto fail_with_errno;
# endif
if (has_width)
@@ -5455,13 +5330,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
errno = EINVAL;
}
- if (result != resultbuf)
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
-
- return NULL;
+ goto fail_with_errno;
}
#if USE_SNPRINTF
@@ -5595,14 +5464,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
NULL,
NULL, &tmpdst_len);
if (tmpdst == NULL)
- {
- if (result != resultbuf)
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
- return NULL;
- }
+ goto fail_with_errno;
ENSURE_ALLOCATION_ELSE (xsum (length, tmpdst_len),
{ free (tmpdst); goto out_of_memory; });
DCHAR_CPY (result + length, tmpdst, tmpdst_len);
@@ -5827,25 +5689,40 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
#if USE_SNPRINTF
overflow:
- if (result != resultbuf)
- free (result);
- if (buf_malloced != NULL)
- free (buf_malloced);
- CLEANUP ();
errno = EOVERFLOW;
- return NULL;
+ goto fail_with_errno;
#endif
out_of_memory:
+ errno = ENOMEM;
+ goto fail_with_errno;
+
+#if ENABLE_UNISTDIO || ((!USE_SNPRINTF || !HAVE_SNPRINTF_RETVAL_C99 || USE_MSVC__SNPRINTF || (NEED_PRINTF_DIRECTIVE_LS && !defined IN_LIBINTL) || ENABLE_WCHAR_FALLBACK) && HAVE_WCHAR_T)
+ fail_with_EILSEQ:
+ errno = EILSEQ;
+ goto fail_with_errno;
+#endif
+
+ fail_with_errno:
if (result != resultbuf)
free (result);
if (buf_malloced != NULL)
free (buf_malloced);
- out_of_memory_1:
CLEANUP ();
- errno = ENOMEM;
return NULL;
}
+
+ out_of_memory_1:
+ errno = ENOMEM;
+ goto fail_1_with_errno;
+
+ fail_1_with_EINVAL:
+ errno = EINVAL;
+ goto fail_1_with_errno;
+
+ fail_1_with_errno:
+ CLEANUP ();
+ return NULL;
}
#undef MAX_ROOM_NEEDED
--
2.25.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: malloc failing with EAGAIN
2022-05-01 20:28 ` malloc failing with EAGAIN Bruno Haible
@ 2022-05-01 22:17 ` Paul Eggert
0 siblings, 0 replies; 12+ messages in thread
From: Paul Eggert @ 2022-05-01 22:17 UTC (permalink / raw
To: Bruno Haible; +Cc: bug-gnulib, Bjarni Ingi Gislason
On 5/1/22 13:28, Bruno Haible wrote:
> I would argue that glibc should use a different errno value in this case.
Either errno value makes sense to me. If you keep doing a non-blocking
read on a pipe that only you can write to you'll keep getting EAGAIN,
which has the same feel as a bug where you grab a lock and then keep
doing a malloc that won't work until you release the lock.
No big deal either way of course.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: vasnprintf.c: "out_of_memory", -Wanalyzer-free-of-non-heap, -Wanalyzer-malloc-leak
2022-05-01 21:28 ` vasnprintf.c: "out_of_memory", -Wanalyzer-free-of-non-heap, -Wanalyzer-malloc-leak Bruno Haible
@ 2022-05-01 22:54 ` Paul Eggert
0 siblings, 0 replies; 12+ messages in thread
From: Paul Eggert @ 2022-05-01 22:54 UTC (permalink / raw
To: Bruno Haible; +Cc: bug-gnulib, Bjarni Ingi Gislason
On 5/1/22 14:28, Bruno Haible wrote:
> I pushed these three patches in your name. (I hope that's fine with you.)
Thanks, looks good.
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-05-01 22:54 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-04-30 14:13 vasnprintf.c: "out_of_memory", -Wanalyzer-free-of-non-heap, -Wanalyzer-malloc-leak Bjarni Ingi Gislason
2022-04-30 20:27 ` Paul Eggert
2022-05-01 17:16 ` Bjarni Ingi Gislason
2022-05-01 18:48 ` Paul Eggert
2022-04-30 21:11 ` Bruno Haible
2022-04-30 23:02 ` Paul Eggert
2022-05-01 0:24 ` Bruno Haible
2022-05-01 2:47 ` Paul Eggert
2022-05-01 20:28 ` malloc failing with EAGAIN Bruno Haible
2022-05-01 22:17 ` Paul Eggert
2022-05-01 21:28 ` vasnprintf.c: "out_of_memory", -Wanalyzer-free-of-non-heap, -Wanalyzer-malloc-leak Bruno Haible
2022-05-01 22:54 ` Paul Eggert
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).