Re: Detecting use-after-free bugs using gcc's malloc() attribute

Hi,

On 2023-06-28 10:40:22 +0200, Peter Eisentraut wrote:
> On 26.06.23 21:54, Andres Freund wrote:
> > For something like pg_list.h the malloc(free) attribute is a bit awkward to
> > use, because one a) needs to list ~30 functions that can free a list and b)
> > the referenced functions need to be declared.
>
> Hmm. Saying list_concat() "deallocates" a list is mighty confusing because
> 1) it doesn't, and 2) it might actually allocate a new list.

list_concat() basically behaves like realloc(), except that the "pointer is
still valid" case is much more common. And the way that's modelled in the
annotations is to say a function frees and allocates.

Note that the free attribute references the first element for list_concat(),
not the second.

> So while you get the useful behavior of "you probably didn't mean to use
> this variable again after passing it into list_concat()", if some other tool
> actually took these allocate/deallocate decorations at face value and did a
> memory leak analysis with them, they'd get completely bogus results.

How would the annotations possibly lead to a bogus result? I see neither how
it could lead to false negatives nor false positives?

The gcc attributes are explicitly intended to track not just plain memory
allocations, the example in the docs
https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes
is to add them for fopen() etc. So I don't think it's likely that external
tools will interpret this is a much more stringent way.

> > I also added such attributes to bitmapset.h and palloc() et al, but that
> > didn't find existing bugs. It does find use-after-free instances if I add
> > some, similarly it does find cases of mismatching palloc with free etc.
>
> This seems more straightforward. Even if it didn't find any bugs, I'd
> imagine it would be useful during development.

Agreed. Given our testing regimen (valgrind etc), I'd expect to find many such
bugs before long in the tree anyway. But it's much nicer to get that far. And
to find paths that aren't covered by tests.

> > Do others think this would be useful enough to be worth polishing? And do you
> > agree the warnings above are bugs?
>
> I actually just played with this the other day, because I can never remember
> termPQExpBuffer() vs. destroyPQExpBuffer().

That's a pretty nasty one :(

> I couldn't quite make it work for that, but I found the feature overall
> useful, so I'd welcome support for it.

Yea, I don't think the attributes can comfortable handle initPQExpBuffer()
style allocation. It's somewhat posible by moving the allocation to an inline
function, and then making the thing that's allocated ->data. But it ends up
pretty messy, particularly because we need ABI stability for pqexpbuffer.h.

But createPQExpBuffer() can be dealt with reasonably.

Doing so points out:

[51/354 42 14%] Compiling C object src/bin/initdb/initdb.p/initdb.c.o
../../../../home/andres/src/postgresql/src/bin/initdb/initdb.c: In function ‘replace_guc_value’:
../../../../home/andres/src/postgresql/src/bin/initdb/initdb.c:566:9: warning: ‘free’ called on pointer returned from a mismatched allocation function [-Wmismatched-dealloc]
566 | free(newline); /* but don't free newline->data */
| ^~~~~~~~~~~~~
../../../../home/andres/src/postgresql/src/bin/initdb/initdb.c:470:31: note: returned from ‘createPQExpBuffer’
470 | PQExpBuffer newline = createPQExpBuffer();
| ^~~~~~~~~~~~~~~~~~~

which is intentional, but ... not pretty, and could very well be a bug in
other cases. If we want to do stuff like that, we'd probably better off
having a dedicated version of destroyPQExpBuffer(). Although here it looks
like the code should just use an on-stack PQExpBuffer.

Greetings,

Andres Freund