Re: vasnprintf.c: "out_of_memory", -Wanalyzer-free-of-non-heap, -Wanalyzer-malloc-leak

[Bruno Haible]

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