On Sat, Apr 06, 2024 at 07:45:17PM +0200, René Scharfe wrote:
> I calmed down a bit now. And ask myself how widespread the issue actually
> is. Used the following silly Coccinelle rule to find functions that use
> errno after a free(3) call:
>
> @@
> @@
> - free(...);
> ...
> errno
>
> Found only a handful of places, and they all set errno explicitly, so
> they are fine.
Is that enough, though? Imagine I have:
int foo(const char *x)
{
char *y = xstfrmt("something/%s", x);
int fd = open(y, ...);
free(y);
return fd;
}
Then errno is important if some caller of foo() checks errno after foo()
returns an error. And that caller might not even be in the same file.
In other words, it really depends on the contract of foo() with respect
to errno. And I don't think there is a way in C to specify that
contract in away that the compiler can understand.
> No idea how to match any use of errno except assignment. And no idea how
> to find indirect callers of free(3) that use errno with no potential
> assignment in between.
Yeah, I guess the indirect callers of free() are really the flip-side.
My example was indirect users of errno. ;)
> > The other reason is that macros (especially of system names) can create
> > their own headaches. We could require xfree() everywhere as a
> > complement to xmalloc (or maybe even just places where the errno
> > preservation seems useful), but that's one more thing to remember.
>
> An xfree() to go along with xmalloc()/xrealloc()/xcalloc()/xstrdup() would
> fit in nicely and might be easier to remember than free() after a while.
> Having to convert thousands of calls is unappealing, though.
My biggest concern with it is that we'd have to remember to use it, and
there's not good compiler enforcement. But I guess coccinelle can help
us there.
My secondary concern is that it might make people think that xmalloc()
and xfree() are always paired, and thus you can do clever things in one
as long as the other matches it. But we sometimes free memory from
system routines like getline(). Maybe a scary comment would be enough?
> Found four places that did not expect free(3) to mess up their errno by
> running the test suite with that. Patch below.
These are perhaps worth fixing (though not if we come up with a
universal solution). But I'd be surprised if they are the only ones. By
its nature, this problem only manifests when there are actual errors,
and our test suite is mostly checking happy paths. So I'd assume there
are a ton of "if (ret < 0) { free(foo); return -1; }" spots that are
simply not exercised by the test suite at all.
-Peff
