From: Jeff King <peff@peff.net>
To: "René Scharfe" <l.s.r@web.de>
Cc: Junio C Hamano <gitster@pobox.com>, Git List <git@vger.kernel.org>
Subject: Re: free and errno, was Re: [PATCH] apply: replace mksnpath() with a mkpathdup() call
Date: Sat, 6 Apr 2024 21:18:34 -0400 [thread overview]
Message-ID: <20240407011834.GA1085004@coredump.intra.peff.net> (raw)
In-Reply-To: <0fc77134-94fb-4d60-95b4-509c3582e20f@web.de>
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
next prev parent reply other threads:[~2024-04-07 1:18 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-04 21:08 [PATCH] apply: replace mksnpath() with a mkpathdup() call René Scharfe
2024-04-04 21:29 ` Junio C Hamano
2024-04-04 22:53 ` free and errno, was " Jeff King
2024-04-04 23:08 ` Junio C Hamano
2024-04-05 10:52 ` René Scharfe
2024-04-05 17:35 ` Jeff King
2024-04-05 17:41 ` Jeff King
2024-04-06 17:45 ` René Scharfe
2024-04-07 1:18 ` Jeff King [this message]
2024-04-14 15:17 ` René Scharfe
2024-04-24 1:11 ` Jeff King
2024-04-05 10:53 ` [PATCH v2 1/2] apply: avoid fixed-size buffer in create_one_file() René Scharfe
2024-04-05 10:56 ` [PATCH v2 2/2] path: remove mksnpath() René Scharfe
2024-04-05 17:37 ` Jeff King
2024-04-05 16:51 ` [PATCH v2 1/2] apply: avoid fixed-size buffer in create_one_file() Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240407011834.GA1085004@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=l.s.r@web.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).