Re: [PATCH] free: preserve errno [BZ#17924]

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 29/12/2020 10:38, H.J. Lu wrote:
> On Mon, Dec 28, 2020 at 11:24 AM Adhemerval Zanella via Libc-alpha
> <libc-alpha@sourceware.org> wrote:
>>
>>
>>
>> On 23/12/2020 22:03, Paul Eggert wrote:
>>> Thanks for the comments about the patch's test case. I modified the test case to reflect nearly all the comments, resulting in the attached revised patch. I'm replying below only to the comments that didn't result in a change to the patch.
>>>
>>> On 12/23/20 11:19 AM, Adhemerval Zanella wrote:
>>>
>>>> Fixing in a more fine grained would require a lot of more work to check if
>>>> the shared routines that calls mmap, madvise or brk won't interfere with other
>>>> symbols; so maybe it should be ok to use this large hammer for now.
>>>
>>> Yes, that was my thought as well.
>>
>> I will try to revise this for 2.33.
>>
>>>
>>>>> +The @code{free} function preserves the value of @code{errno}, so that
>>>>> +cleanup code need not worry about saving and restoring @code{errno}
>>>>> +around a call to @code{free}.  Although neither @w{ISO C} nor
>>>>> +POSIX.1-2017 requires @code{free} to preserve @code{errno}, a future
>>>>> +version of POSIX is planned to require it.
>>> ...
>>>> Not sure if this is worth to add, since we will need to update the manual
>>>> once the POSIX does require it.
>>>
>>> I'll volunteer to update the manual. :-)
>>>
>>> It's worth mentioning that preserving errno is not something that portable C or POSIX code should assume for 'free'. If there's a better way for the manual to warn its readers about this, that'd be fine of course.
>>>
>>> I did consider changing "Although neither @w{ISO C} nor POSIX.1-2017 requires @code{free} to preserve @code{errno}, a future version of POSIX is planned to require it" to "Neither @w{ISO C} nor POSIX requires @code{free} to preserve @code{errno}", but that wording would be less informative and would still need updating once POSIX does require 'free' to preserve errno.
>>
>> Thanks.
>>
>>> From afbf4ff042cf3a5c8f983d5aa3bd0de3fb696dd3 Mon Sep 17 00:00:00 2001
>>> From: Paul Eggert <eggert@cs.ucla.edu>
>>> Date: Wed, 23 Dec 2020 11:27:25 -0800
>>> Subject: [PATCH] free: preserve errno [BZ#17924]
>>>
>>> In the next release of POSIX, free must preserve errno
>>> <https://www.austingroupbugs.net/view.php?id=385>.
>>> Modify __libc_free to save and restore errno, so that
>>> any internal munmap etc. syscalls do not disturb the caller's errno.
>>> Add a test malloc/tst-free-errno.c (almost all by Bruno Haible),
>>> and document that free preserves errno.
>>
>> LGTM with the small test change below.
>>
>> Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
>>
> 
> On a machine with 192 GB RAM, I got
> 
> [hjl@gnu-clx-1 build-x86_64-linux]$ cat malloc/tst-free-errno-mcheck.out
> error: tst-free-errno.c:124: not true: get_errno () == 1789
> error: 1 test failures
> [hjl@gnu-clx-1 build-x86_64-linux]$
> 

We need to fix it for the malloc check hooks as well:

diff --git a/malloc/hooks.c b/malloc/hooks.c
index 6474ba8b38..336ff497e9 100644
--- a/malloc/hooks.c
+++ b/malloc/hooks.c
@@ -260,6 +260,8 @@ free_check (void *mem, const void *caller)
   if (!mem)
     return;
 
+  int err = errno;
+
 #ifdef USE_MTAG
   /* Quickly check that the freed pointer matches the tag for the memory.
      This gives a useful double-free detection.  */
@@ -274,12 +276,16 @@ free_check (void *mem, const void *caller)
     {
       __libc_lock_unlock (main_arena.mutex);
       munmap_chunk (p);
-      return;
     }
-  /* Mark the chunk as belonging to the library again.  */
-  (void)TAG_REGION (chunk2rawmem (p), CHUNK_AVAILABLE_SIZE (p) - CHUNK_HDR_SZ);
-  _int_free (&main_arena, p, 1);
-  __libc_lock_unlock (main_arena.mutex);
+  else
+    {
+      /* Mark the chunk as belonging to the library again.  */
+      (void)TAG_REGION (chunk2rawmem (p), CHUNK_AVAILABLE_SIZE (p)
+                                         - CHUNK_HDR_SZ);
+      _int_free (&main_arena, p, 1);
+      __libc_lock_unlock (main_arena.mutex);
+    }
+  __set_errno (err);
 }
 
 static void *