Re: [PATCH] add support for -Wmismatched-dealloc

[Martin Sebor <msebor@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3017 bytes --]

On 12/14/20 6:01 PM, Joseph Myers wrote:
> On Mon, 14 Dec 2020, Martin Sebor via Libc-alpha wrote:
> 
>>> I spent some time working around this but in the end it turned out
>>> to be too convoluted so I decided to make the attribute a little
>>> smarter.  Instead of associating all allocation functions with all
>>> deallocation functions (such as fdopen, fopen, fopen64, etc. with
>>> fclose, freopen, and freopen64) I changed it so that an allocator
>>> only needs to be associated with a single deallocator (a reallocator
>>> also needs to be associated with itself).  That makes things quite
>>> a bit simpler.
> [...]
>> The GCC patches have now been committed and the dependency resolved.
> 
> I've looked at the attribute documentation now in GCC, but I'm afraid I'm
> unable to understand from that documentation why the proposed glibc patch
> constitutes a valid way of specifying that, for example, it's valid to use
> freopen as a deallocator for FILE pointers opened by functions whose
> attribute only mentions fclose.  Unless there's something I'm missing in
> the documentation or a separate documentation patch that's not yet
> committed, I think more work is needed on the GCC documentation to make
> clear the semantics the glibc patch is asserting for valid combinations of
> allocators and deallocators, so that those semantics can be reviewed for
> correctness.

I flip-flopped with freopen.  Initially I wanted to mark it up as
both an allocator and a deallocator, analogously to realloc (which
is implicitly both) or reallocarray (which is annotated as both in
the latest Glibc patch).  Both the initial Glibc and GCC patches
(the manual for the latter) reflected this and had freopen annotated
that way.

But because freopen doesn't actually deallocate or allocate a stream
the markup wouldn't be correct.  It would cause false positives with
-Wmismatched-dealloc as well with other warnings like the future
-Wuse-after-free (or with -Wanalyzer-use-after-free when the GCC
analyzer adds support for the attribute that David Malcolm is
working on for GCC 11).  I've added a test case to the test suite:

   void f (FILE *f1)
   {
     FILE *f2 = freopen ("", "", f1);
     fclose (f1);   // must not warn
   }

To answer your question, without the attribute freopen is seen by
GCC as an ordinary function that happens to take a FILE* and return
another FILE*.  It neither allocates it nor deallocates it.  For
GCC 12, I'd like us to consider adding attribute returns_arg(position)
to improve the analysis here.  The GCC manual also doesn't mention
freopen anymore but I'd be happy to change the example there to
show an API that does include a reallocator (e.g., reallocarray).

Having said all this, after double-checking the latest Glibc patch
I see it still has the attribute on freopen by mistake (as well as
the ordinary attribute malloc, which would make it even worse).
I've removed both in the attached revision.  Sorry if this confused
you -- freopen obviously confused me.

Martin

[-- Attachment #2: glibc-attr-malloc.diff --]
[-- Type: text/x-patch, Size: 10913 bytes --]

diff --git a/libio/stdio.h b/libio/stdio.h
index 998470943e..a52324cc23 100644
--- a/libio/stdio.h
+++ b/libio/stdio.h
@@ -165,22 +165,31 @@ extern int renameat2 (int __oldfd, const char *__old, int __newfd,
 		      const char *__new, unsigned int __flags) __THROW;
 #endif
 
+/* Close STREAM.
+
+   This function is a possible cancellation point and therefore not
+   marked with __THROW.  */
+extern int fclose (FILE *__stream);
+
 /* Create a temporary file and open it read/write.
 
    This function is a possible cancellation point and therefore not
    marked with __THROW.  */
 #ifndef __USE_FILE_OFFSET64
-extern FILE *tmpfile (void) __wur;
+extern FILE *tmpfile (void)
+  __attribute_malloc__ __attr_dealloc (fclose, 1) __wur;
 #else
 # ifdef __REDIRECT
-extern FILE *__REDIRECT (tmpfile, (void), tmpfile64) __wur;
+extern FILE *__REDIRECT (tmpfile, (void), tmpfile64)
+  __attribute_malloc__ __attr_dealloc (fclose, 1) __wur;
 # else
 #  define tmpfile tmpfile64
 # endif
 #endif
 
 #ifdef __USE_LARGEFILE64
-extern FILE *tmpfile64 (void) __wur;
+extern FILE *tmpfile64 (void)
+   __attribute_malloc__ __attr_dealloc (fclose, 1) __wur;
 #endif
 
 /* Generate a temporary filename.  */
@@ -202,15 +211,17 @@ extern char *tmpnam_r (char *__s) __THROW __wur;
    P_tmpdir is tried and finally "/tmp".  The storage for the filename
    is allocated by `malloc'.  */
 extern char *tempnam (const char *__dir, const char *__pfx)
-     __THROW __attribute_malloc__ __wur;
+  __attribute_malloc__ __wur
+# ifdef __has_builtin
+/* Reference the builtin since the ordinary declaration need not be
+   in scope at this point.  */
+#   if __has_builtin (__builtin_free)
+      __attr_dealloc (__builtin_free, 1)
+#   endif
+# endif
+  __THROW;
 #endif
 
-
-/* Close STREAM.
-
-   This function is a possible cancellation point and therefore not
-   marked with __THROW.  */
-extern int fclose (FILE *__stream);
 /* Flush STREAM, or all streams if STREAM is NULL.
 
    This function is a possible cancellation point and therefore not
@@ -244,7 +255,8 @@ extern int fcloseall (void);
    This function is a possible cancellation point and therefore not
    marked with __THROW.  */
 extern FILE *fopen (const char *__restrict __filename,
-		    const char *__restrict __modes) __wur;
+		    const char *__restrict __modes)
+  __attribute_malloc__ __attr_dealloc (fclose, 1) __wur;
 /* Open a file, replacing an existing stream with it.
 
    This function is a possible cancellation point and therefore not
@@ -256,7 +268,7 @@ extern FILE *freopen (const char *__restrict __filename,
 # ifdef __REDIRECT
 extern FILE *__REDIRECT (fopen, (const char *__restrict __filename,
 				 const char *__restrict __modes), fopen64)
-  __wur;
+  __attribute_malloc__ __attr_dealloc (fclose, 1) __wur;
 extern FILE *__REDIRECT (freopen, (const char *__restrict __filename,
 				   const char *__restrict __modes,
 				   FILE *__restrict __stream), freopen64)
@@ -268,7 +280,8 @@ extern FILE *__REDIRECT (freopen, (const char *__restrict __filename,
 #endif
 #ifdef __USE_LARGEFILE64
 extern FILE *fopen64 (const char *__restrict __filename,
-		      const char *__restrict __modes) __wur;
+		      const char *__restrict __modes)
+  __attribute_malloc__ __attr_dealloc (fclose, 1) __wur;
 extern FILE *freopen64 (const char *__restrict __filename,
 			const char *__restrict __modes,
 			FILE *__restrict __stream) __wur;
@@ -276,7 +289,8 @@ extern FILE *freopen64 (const char *__restrict __filename,
 
 #ifdef	__USE_POSIX
 /* Create a new stream that refers to an existing system file descriptor.  */
-extern FILE *fdopen (int __fd, const char *__modes) __THROW __wur;
+extern FILE *fdopen (int __fd, const char *__modes) __THROW
+  __attribute_malloc__ __attr_dealloc (fclose, 1) __wur;
 #endif
 
 #ifdef	__USE_GNU
@@ -284,18 +298,20 @@ extern FILE *fdopen (int __fd, const char *__modes) __THROW __wur;
    and uses the given functions for input and output.  */
 extern FILE *fopencookie (void *__restrict __magic_cookie,
 			  const char *__restrict __modes,
-			  cookie_io_functions_t __io_funcs) __THROW __wur;
+			  cookie_io_functions_t __io_funcs) __THROW
+  __attribute_malloc__ __attr_dealloc (fclose, 1) __wur;
 #endif
 
 #if defined __USE_XOPEN2K8 || __GLIBC_USE (LIB_EXT2)
 /* Create a new stream that refers to a memory buffer.  */
 extern FILE *fmemopen (void *__s, size_t __len, const char *__modes)
-  __THROW __wur;
+  __THROW __attribute_malloc__ __attr_dealloc (fclose, 1) __wur;
 
 /* Open a stream that writes into a malloc'd buffer that is expanded as
    necessary.  *BUFLOC and *SIZELOC are updated with the buffer's location
    and the number of characters written on fflush or fclose.  */
-extern FILE *open_memstream (char **__bufloc, size_t *__sizeloc) __THROW __wur;
+extern FILE *open_memstream (char **__bufloc, size_t *__sizeloc) __THROW
+  __attribute_malloc__ __attr_dealloc (fclose, 1) __wur;
 #endif
 
 
@@ -792,17 +808,19 @@ extern int fileno_unlocked (FILE *__stream) __THROW __wur;
 
 
 #ifdef __USE_POSIX2
-/* Create a new stream connected to a pipe running the given command.
+/* Close a stream opened by popen and return the status of its child.
 
    This function is a possible cancellation point and therefore not
    marked with __THROW.  */
-extern FILE *popen (const char *__command, const char *__modes) __wur;
+extern int pclose (FILE *__stream);
 
-/* Close a stream opened by popen and return the status of its child.
+/* Create a new stream connected to a pipe running the given command.
 
    This function is a possible cancellation point and therefore not
    marked with __THROW.  */
-extern int pclose (FILE *__stream);
+extern FILE *popen (const char *__command, const char *__modes)
+  __attribute_malloc__ __attr_dealloc (pclose, 1) __wur;
+
 #endif
 
 
diff --git a/libio/tst-freopen.c b/libio/tst-freopen.c
index baa13166fe..a69e4430a7 100644
--- a/libio/tst-freopen.c
+++ b/libio/tst-freopen.c
@@ -81,6 +81,28 @@ do_test_basic (void)
   fclose (f);
 }
 
+/* Verify that freopen returns stream.  */
+static void
+do_test_return_stream (void)
+{
+  FILE *f1 = fopen (name, "r");
+  if (f1 == NULL)
+    FAIL_EXIT1 ("fopen: %m");
+
+  FILE *f2 = freopen (name, "r+", f1);
+  if (f2 == NULL)
+    FAIL_EXIT1 ("freopen: %m");
+
+  /* Verify that freopen isn't declared with attribute malloc
+     (which would let GCC fold the inequality to false).  */
+  if (f1 != f2)
+    FAIL_EXIT1 ("freopen returned a different stream");
+
+  /* This shouldn't trigger -Wmismatched-dealloc.  */
+  fclose (f1);
+}
+
+
 /* Test for BZ#21398, where it tries to freopen stdio after the close
    of its file descriptor.  */
 static void
@@ -105,6 +127,7 @@ do_test (void)
 {
   do_test_basic ();
   do_test_bz21398 ();
+  do_test_return_stream ();
 
   return 0;
 }
diff --git a/libio/tst-popen1.c b/libio/tst-popen1.c
index bae6615b9b..9f36b20090 100644
--- a/libio/tst-popen1.c
+++ b/libio/tst-popen1.c
@@ -21,7 +21,7 @@ do_test (void)
 	  res = 1;
 	}
 
-      fclose (fp);
+      pclose (fp);
     }
 
   fp = popen ("echo hello", "re");
@@ -39,7 +39,7 @@ do_test (void)
 	  res = 1;
 	}
 
-      fclose (fp);
+      pclose (fp);
     }
 
   return res;
diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h
index a06f1cfd91..092e078ef8 100644
--- a/misc/sys/cdefs.h
+++ b/misc/sys/cdefs.h
@@ -560,6 +560,15 @@ _Static_assert (0, "IEEE 128-bits long double requires redirection on this platf
 #  define __attr_access(x)
 #endif
 
+#if __GNUC_PREREQ (11, 0)
+/* Designates dealloc as a function to call to deallocate objects
+   allocated by the declared function.  */
+# define __attr_dealloc(dealloc, argno) \
+    __attribute__ ((__malloc__ (dealloc, argno)))
+#else
+# define __attr_dealloc(x)
+#endif
+
 /* Specify that a function such as setjmp or vfork may return
    twice.  */
 #if __GNUC_PREREQ (4, 1)
diff --git a/stdlib/stdlib.h b/stdlib/stdlib.h
index 3aa27a9d25..a039f17d97 100644
--- a/stdlib/stdlib.h
+++ b/stdlib/stdlib.h
@@ -550,6 +550,9 @@ extern void *calloc (size_t __nmemb, size_t __size)
 extern void *realloc (void *__ptr, size_t __size)
      __THROW __attribute_warn_unused_result__ __attribute_alloc_size__ ((2));
 
+/* Free a block allocated by `malloc', `realloc' or `calloc'.  */
+extern void free (void *__ptr) __THROW;
+
 #ifdef __USE_MISC
 /* Re-allocate the previously allocated block in PTR, making the new
    block large enough for NMEMB elements of SIZE bytes each.  */
@@ -558,11 +561,13 @@ extern void *realloc (void *__ptr, size_t __size)
    between objects pointed by the old and new pointers.  */
 extern void *reallocarray (void *__ptr, size_t __nmemb, size_t __size)
      __THROW __attribute_warn_unused_result__
-     __attribute_alloc_size__ ((2, 3));
-#endif
+     __attribute_alloc_size__ ((2, 3))
+    __attr_dealloc (free, 1);
 
-/* Free a block allocated by `malloc', `realloc' or `calloc'.  */
-extern void free (void *__ptr) __THROW;
+/* Add reallocarray as its own deallocator.  */
+extern void *reallocarray (void *__ptr, size_t __nmemb, size_t __size)
+     __attr_dealloc (reallocarray, 1);
+#endif
 
 #ifdef __USE_MISC
 # include <alloca.h>
@@ -788,7 +793,8 @@ extern int system (const char *__command) __wur;
 /* Return a malloc'd string containing the canonical absolute name of the
    existing named file.  */
 extern char *canonicalize_file_name (const char *__name)
-     __THROW __nonnull ((1)) __wur;
+     __THROW __nonnull ((1)) __attribute_malloc__
+     __attr_dealloc (free, 1) __wur;
 #endif
 
 #if defined __USE_MISC || defined __USE_XOPEN_EXTENDED
@@ -798,7 +804,8 @@ extern char *canonicalize_file_name (const char *__name)
    ENAMETOOLONG; if the name fits in fewer than PATH_MAX chars,
    returns the name in RESOLVED.  */
 extern char *realpath (const char *__restrict __name,
-		       char *__restrict __resolved) __THROW __wur;
+		       char *__restrict __resolved) __THROW
+     __attr_dealloc (free, 1) __wur;
 #endif
 
 
diff --git a/wcsmbs/wchar.h b/wcsmbs/wchar.h
index 9cf8b05a87..7442c80753 100644
--- a/wcsmbs/wchar.h
+++ b/wcsmbs/wchar.h
@@ -562,9 +562,18 @@ extern wchar_t *wcpncpy (wchar_t *__restrict __dest,
 /* Wide character I/O functions.  */
 
 #if defined __USE_XOPEN2K8 || __GLIBC_USE (LIB_EXT2)
+#  ifdef __REDIRECT
+/* Declare the __fclose alias and associate it as a deallocator
+   with open_memstream below.  */
+extern char *__REDIRECT (__fclose, (FILE *), fclose);
+#    define __attr_dealloc_fclose __attr_dealloc (__fclose, 1)
+#  else
+#    define __attr_dealloc_fclose /* empty */
+#  endif
 /* Like OPEN_MEMSTREAM, but the stream is wide oriented and produces
    a wide character string.  */
-extern __FILE *open_wmemstream (wchar_t **__bufloc, size_t *__sizeloc) __THROW;
+extern __FILE *open_wmemstream (wchar_t **__bufloc, size_t *__sizeloc) __THROW
+  __attribute_malloc__ __attr_dealloc_fclose;
 #endif
 
 #if defined __USE_ISOC95 || defined __USE_UNIX98