Re: free and errno, was Re: [PATCH] apply: replace mksnpath() with a mkpathdup() call

From: René Scharfe <l.s.r@web.de>
Date: Apr 6, 2024, 17:45 UTC
Message-ID: <0fc77134-94fb-4d60-95b4-509c3582e20f@web.de>
In-Reply-To: <20240405173517.GA2529133@coredump.intra.peff.net>

Am 05.04.24 um 19:35 schrieb Jeff King:

Show 29 quoted lines

> On Fri, Apr 05, 2024 at 12:52:35PM +0200, René Scharfe wrote:
>
>>> So would it be that unreasonable to assume the modern, desired behavior,
>>> and mostly shrug our shoulders for other platforms? We could even
>>> provide:
>>>
>>>   #ifdef FREE_CLOBBERS_ERRNO
>>>   void git_free(void *ptr)
>>>   {
>>>         int saved_errno = errno;
>>>         free(ptr);
>>>         errno = saved_errno;
>>>   }
>>>   #define free(ptr) git_free(ptr)
>>>   #endif
>>>
>>> if we really wanted to be careful, though it's hard to know which
>>> platforms even need it (and again, it's so rare to come up in practice
>>> that I'd suspect people could go for a long time before realizing their
>>> platform was a problem). I guess the flip side is that we could use the
>>> function above by default, and disable it selectively (the advantage of
>>> disabling it being that it's slightly more efficient; maybe that's not
>>> even measurable?).
>>
>> I think performing this ritual automatically every time on all platforms
>> (i.e. to use git_free() unconditionally) to provide sane errno values
>> around free(3) calls is better than having to worry about attacks from
>> the deep.  But then again I'm easily scared and still a bit in shock, so
>> perhaps I'm overreacting.

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.

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.

Show 11 quoted lines

> You may be right. The main reason not to do it is the extra assignments
> (and call to errno, which I think can be a function hidden behind a
> macro these days). But I kind of doubt it is measurable, and we expect
> malloc/free to be a bit heavyweight (compared to regular instructions)
> anyway. So it is probably me being overly cautious about the performance
> side.
>
> 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.

Show 6 quoted lines

> With the patch below you can see both in action:
>
>   - hyperfine seems to show the git_free() version as ~1% slower to run
>     "git log" (which I picked as something that probably does a
>     reasonable number of mallocs). Frankly, I'm still suspicious that it
>     could have such an impact. Maybe inlining git_free() would help?

I get a slowdown of ca. 0.5%:

Benchmark 1: ./git_2.44.0 log v2.44.0
  Time (mean ± σ):     705.8 ms ±   1.7 ms    [User: 674.0 ms, System: 28.3 ms]
  Range (min … max):   702.3 ms … 709.2 ms    10 runs

Benchmark 2: ./git log v2.44.0
  Time (mean ± σ):     708.1 ms ±   2.0 ms    [User: 676.4 ms, System: 28.7 ms]
  Range (min … max):   705.0 ms … 710.9 ms    10 runs

Hmm.

What if we do the opposite and poison errno in git_free() instead of carrying over the original value? That's only half the work.

Benchmark 1: ./git_2.44.0 log v2.44.0
  Time (mean ± σ):     704.2 ms ±   1.3 ms    [User: 674.2 ms, System: 27.6 ms]
  Range (min … max):   702.2 ms … 705.8 ms    10 runs

Benchmark 2: ./git log v2.44.0
  Time (mean ± σ):     706.3 ms ±   0.9 ms    [User: 676.3 ms, System: 27.5 ms]
  Range (min … max):   704.8 ms … 708.2 ms    10 runs

So that's slightly better.  But the measurements are quite noisy.

Found four places that did not expect free(3) to mess up their errno by running the test suite with that. Patch below.

>   - usually #defining free(ptr) with an argument will avoid confusion
>     with the word "free" as a token. But in this case there's a function
>     pointer which is then called as struct->free(ptr), causing
>     confusion.

In other contexts we use "clear" or "release" to name functions like that.

René

 compat/precompose_utf8.c | 2 ++
 git.c                    | 3 +++
 lockfile.c               | 3 +++
 tempfile.c               | 2 ++
 4 files changed, 10 insertions(+)

diff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c
index 0bd5c24250..4da80b40ce 100644
--- a/compat/precompose_utf8.c
+++ b/compat/precompose_utf8.c
@@ -114,8 +114,10 @@ PREC_DIR *precompose_utf8_opendir(const char *dirname)

 	prec_dir->dirp = opendir(dirname);
 	if (!prec_dir->dirp) {
+		int save_errno = errno;
 		free(prec_dir->dirent_nfc);
 		free(prec_dir);
+		errno = save_errno;
 		return NULL;
 	} else {
 		int ret_errno = errno;
diff --git a/git.c b/git.c
index 654d615a18..275674f873 100644
--- a/git.c
+++ b/git.c
@@ -773,6 +773,7 @@ static int run_argv(int *argcp, const char ***argv)
 	int done_alias = 0;
 	struct string_list cmd_list = STRING_LIST_INIT_NODUP;
 	struct string_list_item *seen;
+	int save_errno;

 	while (1) {
 		/*
@@ -853,7 +854,9 @@ static int run_argv(int *argcp, const char ***argv)
 		done_alias = 1;
 	}

+	save_errno = errno;
 	string_list_clear(&cmd_list, 0);
+	errno = save_errno;

 	return done_alias;
 }
diff --git a/lockfile.c b/lockfile.c
index 1d5ed01682..b8401f5059 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -76,6 +76,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags,
 		     int mode)
 {
 	struct strbuf filename = STRBUF_INIT;
+	int save_errno;

 	strbuf_addstr(&filename, path);
 	if (!(flags & LOCK_NO_DEREF))
@@ -83,7 +84,9 @@ static int lock_file(struct lock_file *lk, const char *path, int flags,

 	strbuf_addstr(&filename, LOCK_SUFFIX);
 	lk->tempfile = create_tempfile_mode(filename.buf, mode);
+	save_errno = errno;
 	strbuf_release(&filename);
+	errno = save_errno;
 	return lk->tempfile ? lk->tempfile->fd : -1;
 }

diff --git a/tempfile.c b/tempfile.c
index ed88cf8431..029b760fa9 100644
--- a/tempfile.c
+++ b/tempfile.c
@@ -144,7 +144,9 @@ struct tempfile *create_tempfile_mode(const char *path, int mode)
 		tempfile->fd = open(tempfile->filename.buf,
 				    O_RDWR | O_CREAT | O_EXCL, mode);
 	if (tempfile->fd < 0) {
+		int save_errno = errno;
 		deactivate_tempfile(tempfile);
+		errno = save_errno;
 		return NULL;
 	}
 	activate_tempfile(tempfile);