[fortran] PR28105 Overflow check for memory allocation

Submitted by Janne Blomqvist on Dec. 13, 2010, 9:06 p.m.

Details

Message ID AANLkTin=anKwViM_6s6PCBCM+rgEJg3PS9M16YfC5jpO@mail.gmail.com
State New
Headers show

Commit Message

Janne Blomqvist Dec. 13, 2010, 9:06 p.m.
Hi,

as a result of a recent commit, size_t in the gfortran frontend is now
properly an unsigned type. As a side-effect of this, testing whether a
value of type size_t is negative just generates dead code which is
optimized away. This kind of test was previously used as a crude kind
of overflow check for memory allocation (for the ALLOCATE statement
there is now a proper implementation).

The attached patch removes the generation of this dead code in the first place.

Regtested on x86_64-unknown-linux-gnu, Ok for trunk?

Comments

Steve Kargl Dec. 14, 2010, 6:41 a.m.
On Mon, Dec 13, 2010 at 11:06:08PM +0200, Janne Blomqvist wrote:
> Hi,
> 
> as a result of a recent commit, size_t in the gfortran frontend is now
> properly an unsigned type. As a side-effect of this, testing whether a
> value of type size_t is negative just generates dead code which is
> optimized away. This kind of test was previously used as a crude kind
> of overflow check for memory allocation (for the ALLOCATE statement
> there is now a proper implementation).
> 
> The attached patch removes the generation of this dead code in the first place.
> 
> Regtested on x86_64-unknown-linux-gnu, Ok for trunk?
> 

OK.

Just out of curiousity, your error message is now "Out of memory".
If someone's shell restricts that amount of memory allowed to be
allocate, one could run into an allocation issue even though the
system has memory available.  Won't a better error message be
"memory allocation issue"?
Janne Blomqvist Dec. 15, 2010, 10:18 a.m.
On Tue, Dec 14, 2010 at 08:41, Steve Kargl
<sgk@troutmask.apl.washington.edu> wrote:
> On Mon, Dec 13, 2010 at 11:06:08PM +0200, Janne Blomqvist wrote:
>> Hi,
>>
>> as a result of a recent commit, size_t in the gfortran frontend is now
>> properly an unsigned type. As a side-effect of this, testing whether a
>> value of type size_t is negative just generates dead code which is
>> optimized away. This kind of test was previously used as a crude kind
>> of overflow check for memory allocation (for the ALLOCATE statement
>> there is now a proper implementation).
>>
>> The attached patch removes the generation of this dead code in the first place.
>>
>> Regtested on x86_64-unknown-linux-gnu, Ok for trunk?
>>
>
> OK.

Thanks.

> Just out of curiousity, your error message is now "Out of memory".
> If someone's shell restricts that amount of memory allowed to be
> allocate, one could run into an allocation issue even though the
> system has memory available.  Won't a better error message be
> "memory allocation issue"?

Good point. POSIX specifies that if malloc() returns NULL, errno is
set to ENOMEM which, according to the spec, means "Insufficient
storage space is available."

http://pubs.opengroup.org/onlinepubs/9699919799/functions/malloc.html

This might mean that, yes, the system is out of memory, or it might
mean something else like a per-process or per-user limit preventing
the allocation from succeeding, as you say. Any ideas for a suitable
wording? IMHO the POSIX "Insufficient storage space is available."
error message might confuse a user to believe e.g. that (s)he is out
of disk space or something like that, so I'd suggest "Memory
allocation error" or "Error allocating memory". "Issue" is maybe a bit
vague (did it actually succeed or not?)?

OTOH since we're calling _gfortran_os_error which calls perror() we
get the same message twice just worded slightly differently. E.g.

$ ./largealloc
Operating system error: Cannot allocate memory
Out of memory

which is a bit redundant. So maybe just an empty string (or NULL) is sufficient?
Steve Kargl Dec. 15, 2010, 4:20 p.m.
On Wed, Dec 15, 2010 at 12:18:16PM +0200, Janne Blomqvist wrote:
> 
> OTOH since we're calling _gfortran_os_error which calls perror() we
> get the same message twice just worded slightly differently. E.g.
> 
> $ ./largealloc
> Operating system error: Cannot allocate memory
> Out of memory
> 
> which is a bit redundant. So maybe just an empty string (or NULL)
> is sufficient?
> 

In the above, how about changing "Out of memory" to 
"Allocation would exceed system or user memory limit".
Thus, one would get 

$ ./largealloc
Operating system error: Cannot allocate memory
Allocation would exceed system or user memory limit

Second, provides a hint to user where to look for 
the problem.  Whatever you decide is fine with me.
Janne Blomqvist Dec. 15, 2010, 5:17 p.m.
On Tue, Dec 14, 2010 at 08:41, Steve Kargl
<sgk@troutmask.apl.washington.edu> wrote:
> On Mon, Dec 13, 2010 at 11:06:08PM +0200, Janne Blomqvist wrote:
>> Hi,
>>
>> as a result of a recent commit, size_t in the gfortran frontend is now
>> properly an unsigned type. As a side-effect of this, testing whether a
>> value of type size_t is negative just generates dead code which is
>> optimized away. This kind of test was previously used as a crude kind
>> of overflow check for memory allocation (for the ALLOCATE statement
>> there is now a proper implementation).
>>
>> The attached patch removes the generation of this dead code in the first place.
>>
>> Regtested on x86_64-unknown-linux-gnu, Ok for trunk?
>>
>
> OK.

Sending        fortran/ChangeLog
Sending        fortran/trans.c
Transmitting file data ..
Committed revision 167860.

> Just out of curiousity, your error message is now "Out of memory".

FWIW, the above commit was the original patch, I'll fix the error
messages in a separate commit.

Patch hide | download patch | download mbox

diff --git a/gcc/fortran/trans.c b/gcc/fortran/trans.c
index b84029b..f3914a1 100644
--- a/gcc/fortran/trans.c
+++ b/gcc/fortran/trans.c
@@ -519,7 +519,7 @@  gfc_trans_runtime_check (bool error, bool once, tree cond, stmtblock_t * pblock,
 
 
 /* Call malloc to allocate size bytes of memory, with special conditions:
-      + if size <= 0, return a malloced area of size 1,
+      + if size == 0, return a malloced area of size 1,
       + if malloc returns NULL, issue a runtime error.  */
 tree
 gfc_call_malloc (stmtblock_t * block, tree type, tree size)
@@ -584,37 +584,21 @@  gfc_call_malloc (stmtblock_t * block, tree type, tree size)
       if (stat)
 	*stat = 0;
 
-      // The only time this can happen is the size wraps around.
-      if (size < 0)
+      newmem = malloc (MAX (size, 1));
+      if (newmem == NULL)
       {
-	if (stat)
-	{
-	  *stat = LIBERROR_ALLOCATION;
-	  newmem = NULL;
-	}
-	else
-	  runtime_error ("Attempt to allocate negative amount of memory. "
-			 "Possible integer overflow");
-      }
-      else
-      {
-	newmem = malloc (MAX (size, 1));
-	if (newmem == NULL)
-	{
-	  if (stat)
-	    *stat = LIBERROR_ALLOCATION;
-	  else
-	    runtime_error ("Out of memory");
-	}
+        if (stat)
+          *stat = LIBERROR_ALLOCATION;
+        else
+	  runtime_error ("Out of memory");
       }
-
       return newmem;
     }  */
 tree
 gfc_allocate_with_status (stmtblock_t * block, tree size, tree status)
 {
   stmtblock_t alloc_block;
-  tree res, tmp, error, msg, cond;
+  tree res, tmp, msg, cond;
   tree status_type = status ? TREE_TYPE (TREE_TYPE (status)) : NULL_TREE;
 
   /* Evaluate size only once, and make sure it has the right type.  */
@@ -640,32 +624,6 @@  gfc_allocate_with_status (stmtblock_t * block, tree size, tree status)
       gfc_add_expr_to_block (block, tmp);
     }
 
-  /* Generate the block of code handling (size < 0).  */
-  msg = gfc_build_addr_expr (pchar_type_node, gfc_build_localized_cstring_const
-			("Attempt to allocate negative amount of memory. "
-			 "Possible integer overflow"));
-  error = build_call_expr_loc (input_location,
-			   gfor_fndecl_runtime_error, 1, msg);
-
-  if (status != NULL_TREE && !integer_zerop (status))
-    {
-      /* Set the status variable if it's present.  */
-      stmtblock_t set_status_block;
-
-      gfc_start_block (&set_status_block);
-      gfc_add_modify (&set_status_block,
-		      fold_build1_loc (input_location, INDIRECT_REF,
-				       status_type, status),
-			   build_int_cst (status_type, LIBERROR_ALLOCATION));
-      gfc_add_modify (&set_status_block, res,
-			   build_int_cst (prvoid_type_node, 0));
-
-      tmp = fold_build2_loc (input_location, EQ_EXPR, boolean_type_node,
-			     status, build_int_cst (TREE_TYPE (status), 0));
-      error = fold_build3_loc (input_location, COND_EXPR, void_type_node, tmp,
-			       error, gfc_finish_block (&set_status_block));
-    }
-
   /* The allocation itself.  */
   gfc_start_block (&alloc_block);
   gfc_add_modify (&alloc_block, res,
@@ -703,12 +661,7 @@  gfc_allocate_with_status (stmtblock_t * block, tree size, tree status)
 					  build_int_cst (prvoid_type_node, 0)),
 			 tmp, build_empty_stmt (input_location));
   gfc_add_expr_to_block (&alloc_block, tmp);
-
-  cond = fold_build2_loc (input_location, LT_EXPR, boolean_type_node, size,
-			  build_int_cst (TREE_TYPE (size), 0));
-  tmp = fold_build3_loc (input_location, COND_EXPR, void_type_node, cond, error,
-			 gfc_finish_block (&alloc_block));
-  gfc_add_expr_to_block (block, tmp);
+  gfc_add_expr_to_block (block, gfc_finish_block (&alloc_block));
 
   return res;
 }
@@ -1048,8 +1001,6 @@  gfc_deallocate_scalar_with_status (tree pointer, tree status, bool can_fail,
 void *
 internal_realloc (void *mem, size_t size)
 {
-  if (size < 0)
-    runtime_error ("Attempt to allocate a negative amount of memory.");
   res = realloc (mem, size);
   if (!res && size != 0)
     _gfortran_os_error ("Out of memory");
@@ -1062,7 +1013,7 @@  internal_realloc (void *mem, size_t size)
 tree
 gfc_call_realloc (stmtblock_t * block, tree mem, tree size)
 {
-  tree msg, res, negative, nonzero, zero, null_result, tmp;
+  tree msg, res, nonzero, zero, null_result, tmp;
   tree type = TREE_TYPE (mem);
 
   size = gfc_evaluate_now (size, block);
@@ -1073,17 +1024,6 @@  gfc_call_realloc (stmtblock_t * block, tree mem, tree size)
   /* Create a variable to hold the result.  */
   res = gfc_create_var (type, NULL);
 
-  /* size < 0 ?  */
-  negative = fold_build2_loc (input_location, LT_EXPR, boolean_type_node, size,
-			      build_int_cst (size_type_node, 0));
-  msg = gfc_build_addr_expr (pchar_type_node, gfc_build_localized_cstring_const
-      ("Attempt to allocate a negative amount of memory."));
-  tmp = fold_build3_loc (input_location, COND_EXPR, void_type_node, negative,
-			 build_call_expr_loc (input_location,
-					    gfor_fndecl_runtime_error, 1, msg),
-			 build_empty_stmt (input_location));
-  gfc_add_expr_to_block (block, tmp);
-
   /* Call realloc and check the result.  */
   tmp = build_call_expr_loc (input_location,
 			 built_in_decls[BUILT_IN_REALLOC], 2,