RFC: Handling realloc(0)

Message ID 87v983aeox.fsf@redhat.com
State New
Headers show
Series
  • RFC: Handling realloc(0)
Related show

Commit Message

Nick Clifton via Binutils April 30, 2021, 5:02 p.m.
Hi Guys,

  A recent binutils bug report (PR 27797) has brought to light the fact
  that the behaviour of realloc(0) is not well defined.  So I am
  proposing the attached patch to change the behaviour of the
  bfd_realloc and bfd_realloc_or_free functions.  With this patch
  applied:

    bfd_realloc (NULL, 0)          will return bfd_malloc (1)
    bfd_realloc (ptr, 0)           will return ptr
    bfd_realloc_of_free (NULL, 0)  will return NULL (and no error set)
    bfd_realloc_or_free (ptr, 0)   will return NULL and ptr will have been freed.

  What do people think ?  Any objections ?

Cheers
  Nick

bfd/ChangeLog
2021-04-30  Nick Clifton  <nickc@redhat.com>

	* libbfd.c (bfd_realloc): Provide some documentation.  Treat a
	size of 0 as a size of 1.
	(bfd_realloc_or_free): Provide some documentation.  Treat a size
	of 0 as a request to free.
	* libbfd-in.h (bfd_realloc): Remove prototype.
	(bfd_realloc_or_free): Remove prototype.
	* libbfd.h: Regenerate.

Comments

Nick Clifton via Binutils April 30, 2021, 5:48 p.m. | #1
Am Freitag, 30. April 2021, 19:02:40 MESZ hat Nick Clifton via Binutils <binutils@sourceware.org> Folgendes geschrieben:

> Hi Guys,

>

>   A recent binutils bug report (PR 27797) has brought to light the fact

>   that the behaviour of realloc(0) is not well defined.  So I am

>   proposing the attached patch to change the behaviour of the

>   bfd_realloc and bfd_realloc_or_free functions.  With this patch

>   applied:

>

>     bfd_realloc (NULL, 0)          will return bfd_malloc (1)

>     bfd_realloc (ptr, 0)          will return ptr

>     bfd_realloc_of_free (NULL, 0)  will return NULL (and no error set)

>     bfd_realloc_or_free (ptr, 0)  will return NULL and ptr will have been freed.

>

>   What do people think ?  Any objections ?

>

> Cheers

>   Nick

>

>

> @@ -296,23 +319,59 @@ bfd_realloc (void *ptr, bfd_size_type size)

>        return NULL;

>      }

>  

> +  /* The behaviour of realloc(0) is implementation defined,

> +     but for this function we always allocate memory.  */

> +  if (size == 0)

> +    size = 1;

>    ret = realloc (ptr, sz);


I think you meant to use sz here instead of size.


Regards
Hannes
Howard Chu April 30, 2021, 5:51 p.m. | #2
Nick Clifton via Binutils wrote:
> Hi Guys,

> 

>   A recent binutils bug report (PR 27797) has brought to light the fact

>   that the behaviour of realloc(0) is not well defined.  So I am

>   proposing the attached patch to change the behaviour of the

>   bfd_realloc and bfd_realloc_or_free functions.  With this patch

>   applied:

> 

>     bfd_realloc (NULL, 0)          will return bfd_malloc (1)


Is bfd_malloc(0) also treated as bfd_malloc(1) ?

>     bfd_realloc (ptr, 0)           will return ptr


Shouldn't this return NULL? a standard realloc(ptr, 0) == free(ptr).

>     bfd_realloc_of_free (NULL, 0)  will return NULL (and no error set)

>     bfd_realloc_or_free (ptr, 0)   will return NULL and ptr will have been freed.

> 

>   What do people think ?  Any objections ?

> 

> Cheers

>   Nick

> 

> bfd/ChangeLog

> 2021-04-30  Nick Clifton  <nickc@redhat.com>

> 

> 	* libbfd.c (bfd_realloc): Provide some documentation.  Treat a

> 	size of 0 as a size of 1.

> 	(bfd_realloc_or_free): Provide some documentation.  Treat a size

> 	of 0 as a request to free.

> 	* libbfd-in.h (bfd_realloc): Remove prototype.

> 	(bfd_realloc_or_free): Remove prototype.

> 	* libbfd.h: Regenerate.

> 



-- 
  -- Howard Chu
  CTO, Symas Corp.           http://www.symas.com
  Director, Highland Sun     http://highlandsun.com/hyc/
  Chief Architect, OpenLDAP  http://www.openldap.org/project/
Nick Clifton via Binutils April 30, 2021, 5:56 p.m. | #3
On 30 Apr 2021 18:02, Nick Clifton via Binutils wrote:
>   A recent binutils bug report (PR 27797) has brought to light the fact

>   that the behaviour of realloc(0) is not well defined.  So I am

>   proposing the attached patch to change the behaviour of the

>   bfd_realloc and bfd_realloc_or_free functions.  With this patch

>   applied:

> 

>     bfd_realloc (NULL, 0)          will return bfd_malloc (1)

>     bfd_realloc (ptr, 0)           will return ptr

>     bfd_realloc_of_free (NULL, 0)  will return NULL (and no error set)

>     bfd_realloc_or_free (ptr, 0)   will return NULL and ptr will have been freed.

> 

>   What do people think ?  Any objections ?


afaik, this matches the general GNU behavior (C library & gnulib), so lgtm
-mike
Maciej W. Rozycki April 30, 2021, 7:46 p.m. | #4
On Fri, 30 Apr 2021, Howard Chu wrote:

> >     bfd_realloc (ptr, 0)           will return ptr

> 

> Shouldn't this return NULL? a standard realloc(ptr, 0) == free(ptr).


 Nope, the standard has[1]:

"If the size of the space requested is zero, the behavior is 
implementation-defined: either a null pointer is returned, or the behavior 
is as if the size were some nonzero value, except that the returned 
pointer shall not be used to access an object."

so this approach is perfectly fine.

References:

[1] "Programming languages -- C", INTERNATIONAL STANDARD, ISO/IEC 9899,
    Second edition, 1999-12-01, Section 7.20.3 "Memory management 
    functions", p.312

  Maciej
Nick Clifton via Binutils May 4, 2021, 11:17 a.m. | #5
Hi Hannes,

>> +  if (size == 0)

>> +    size = 1;

>>      ret = realloc (ptr, sz);

> 

> I think you meant to use sz here instead of size.


Doh!  Yes, thanks for the catch.

Cheers
   Nick
Nick Clifton via Binutils May 4, 2021, 11:26 a.m. | #6
Hi Howard,

> Is bfd_malloc(0) also treated as bfd_malloc(1) ?


This is not currently defined, but it should be.  I will update the patch
to enfore bfd_malloc(0) == bfd_malloc(1).

>>      bfd_realloc (ptr, 0)           will return ptr

> 

> Shouldn't this return NULL? a standard realloc(ptr, 0) == free(ptr).


No.  This is the whole point of this patch.  bfd_realloc() only returns NULL
if there is insufficient memory, and it never frees anything.  bfd_realloc_or_free()
on the other hand will free memory if passed a size of 0, and it will return
NULL in this case.

Cheers
   Nick
Nick Clifton via Binutils May 4, 2021, 8:05 p.m. | #7
On 4/30/21 11:02 AM, Nick Clifton via Binutils wrote:
> Hi Guys,

> 

>    A recent binutils bug report (PR 27797) has brought to light the fact

>    that the behaviour of realloc(0) is not well defined.  So I am

>    proposing the attached patch to change the behaviour of the

>    bfd_realloc and bfd_realloc_or_free functions.  With this patch

>    applied:

> 

>      bfd_realloc (NULL, 0)          will return bfd_malloc (1)

>      bfd_realloc (ptr, 0)           will return ptr

>      bfd_realloc_of_free (NULL, 0)  will return NULL (and no error set)

>      bfd_realloc_or_free (ptr, 0)   will return NULL and ptr will have been freed.

> 

>    What do people think ?  Any objections ?


Not an objection, just a suggestion to enable -Walloc-zero which helps
detect zero allocations but is not included in -Wall because it caused
some false positives in GCC itself.  My build with the option and GCC
11 on x86_64-linux doesn't turn up any instances so it might be worth
giving a try in conjunction with this change to detect the same problem
in calls to other functions (malloc, calloc, realloc and those declared
with attribute alloc_size).

Martin

> 

> Cheers

>    Nick

> 

> bfd/ChangeLog

> 2021-04-30  Nick Clifton  <nickc@redhat.com>

> 

> 	* libbfd.c (bfd_realloc): Provide some documentation.  Treat a

> 	size of 0 as a size of 1.

> 	(bfd_realloc_or_free): Provide some documentation.  Treat a size

> 	of 0 as a request to free.

> 	* libbfd-in.h (bfd_realloc): Remove prototype.

> 	(bfd_realloc_or_free): Remove prototype.

> 	* libbfd.h: Regenerate.

>

Patch

diff --git a/bfd/ChangeLog b/bfd/ChangeLog
index c5a617cef97..64d1a8e49e3 100644
--- a/bfd/ChangeLog
+++ b/bfd/ChangeLog
@@ -1,3 +1,11 @@ 
+2021-04-30  Nick Clifton  <nickc@redhat.com>
+
+	* libbfd.c (bfd_realloc): Provide some documentation.  Treat a
+	size of 0 as a size of 1.
+	(bfd_realloc_or_free): Provide some documentation.  Treat a size
+	of 0 as a request to free.
+	* libbfd.h: Regenerate.
+
 2021-04-30  Nick Clifton  <nickc@redhat.com>
 
 	PR 27801
diff --git a/bfd/libbfd-in.h b/bfd/libbfd-in.h
index 1ad1af8c002..298d909c349 100644
--- a/bfd/libbfd-in.h
+++ b/bfd/libbfd-in.h
@@ -110,10 +110,6 @@  struct areltdata
 
 extern void *bfd_malloc
   (bfd_size_type) ATTRIBUTE_HIDDEN;
-extern void *bfd_realloc
-  (void *, bfd_size_type) ATTRIBUTE_HIDDEN;
-extern void *bfd_realloc_or_free
-  (void *, bfd_size_type) ATTRIBUTE_HIDDEN;
 extern void *bfd_zmalloc
   (bfd_size_type) ATTRIBUTE_HIDDEN;
 
diff --git a/bfd/libbfd.c b/bfd/libbfd.c
index 52c924560b2..1289243c01b 100644
--- a/bfd/libbfd.c
+++ b/bfd/libbfd.c
@@ -279,6 +279,29 @@  bfd_malloc (bfd_size_type size)
   return ptr;
 }
 
+/*
+INTERNAL_FUNCTION
+	bfd_realloc
+
+SYNOPSIS
+	extern void * bfd_realloc (void * MEM, bfd_size_type SIZE) ATTRIBUTE_HIDDEN;
+
+DESCRIPTION
+	Returns a pointer to an allocated block of memory that is at least
+	SIZE bytes long.  If SIZE is 0 then it will be treated as if it were
+	1.  If SIZE is too big then NULL will be returned.
+	
+	If MEM is not NULL then it must point to an allocated block of memory.
+	If this block is large enough then MEM may be used as the return
+	value for this function, but this is not guaranteed.
+
+	If MEM is not returned then the first N bytes in the returned block
+	will be identical to the first N bytes in region pointed to by MEM,
+	where N is the lessor of SIZE and the length of the region of memory
+	currently addressed by MEM.
+
+	Returns NULL upon error and sets bfd_error.
+*/
 void *
 bfd_realloc (void *ptr, bfd_size_type size)
 {
@@ -296,23 +319,59 @@  bfd_realloc (void *ptr, bfd_size_type size)
       return NULL;
     }
 
+  /* The behaviour of realloc(0) is implementation defined,
+     but for this function we always allocate memory.  */
+  if (size == 0)
+    size = 1;
   ret = realloc (ptr, sz);
 
-  if (ret == NULL && sz != 0)
+  if (ret == NULL)
     bfd_set_error (bfd_error_no_memory);
 
   return ret;
 }
 
-/* Reallocate memory using realloc.
-   If this fails the pointer is freed before returning.  */
+/*
+INTERNAL_FUNCTION
+	bfd_realloc_or_free
+
+SYNOPSIS
+	extern void * bfd_realloc_or_free (void * MEM, bfd_size_type SIZE) ATTRIBUTE_HIDDEN;
+
+DESCRIPTION
+	Returns a pointer to an allocated block of memory that is at least
+	SIZE bytes long.  If SIZE is 0 then no memory will be allocated,
+	MEM will be freed, and NULL will be returned.  This will not cause
+	bfd_error to be set.
+
+	If SIZE is too big then NULL will be returned and bfd_error will be
+	set. 
+	
+	If MEM is not NULL then it must point to an allocated block of memory.
+	If this block is large enough then MEM may be used as the return
+	value for this function, but this is not guaranteed.
+
+	If MEM is not returned then the first N bytes in the returned block
+	will be identical to the first N bytes in region pointed to by MEM,
+	where N is the lessor of SIZE and the length of the region of memory
+	currently addressed by MEM.
+*/
 
 void *
 bfd_realloc_or_free (void *ptr, bfd_size_type size)
 {
-  void *ret = bfd_realloc (ptr, size);
+  void *ret;
 
-  if (ret == NULL && size > 0)
+  /* The behaviour of realloc(0) is implementation defined, but
+     for this function we treat it is always freeing the memory.  */
+  if (size == 0)
+    {
+      free (ptr);
+      return NULL;
+    }
+      
+  ret = bfd_realloc (ptr, size);
+  if (ret == NULL)
     free (ptr);
 
   return ret;
diff --git a/bfd/libbfd.h b/bfd/libbfd.h
index fba29998900..de37842c6a2 100644
--- a/bfd/libbfd.h
+++ b/bfd/libbfd.h
@@ -966,6 +966,10 @@  _bfd_malloc_and_read (bfd *abfd, bfd_size_type asize, bfd_size_type rsize)
   return NULL;
 }
 /* Extracted from libbfd.c.  */
+void * bfd_realloc (void * MEM, bfd_size_type SIZE);
+
+void * bfd_realloc_or_free (void * MEM, bfd_size_type SIZE);
+
 bool bfd_write_bigendian_4byte_int (bfd *, unsigned int);
 
 unsigned int bfd_log2 (bfd_vma x);
diff --git a/bfd/plugin.c b/bfd/plugin.c
index c4f2be8999e..70151ce97ae 100644
--- a/bfd/plugin.c
+++ b/bfd/plugin.c
@@ -217,7 +217,7 @@  bfd_plugin_open_input (bfd *ibfd, struct ld_plugin_input_file *file)
 
       if (fstat (file->fd, &stat_buf))
 	{
-	  close(file->fd);
+	  close (file->fd);
 	  return 0;
 	}