[PATCH] Small regexec.c fixes

[PATCH] Small regexec.c fixes

[Jakub Jelinek]

Hi!

2003-12-29  Jakub Jelinek  <jakub@redhat.com>

	* posix/regexec.c (re_copy_regs): Revert comment change.
	Avoid memory leak if realloc fails.
	(proceed_next_node): Return -2 if re_node_set_insert fails.
	Return -2 if push_fail_stack fails.
	(push_fail_stack): Change fs->alloc only after successful
	realloc.
	(pop_fail_stack): Formatting.
	(set_regs): If proceed_next_node returns -2, free eps_via_nodes
	and fs.
	(check_arrival_add_next_nodes): Merge identical statements
	from if branches.

--- libc/posix/regexec.c.jj	2003-12-29 15:03:54.000000000 +0100
+++ libc/posix/regexec.c	2003-12-29 15:23:07.000000000 +0100
@@ -453,8 +453,7 @@ re_copy_regs (regs, pmatch, nregs, regs_
 
   /* Have the register data arrays been allocated?  */
   if (regs_allocated == REGS_UNALLOCATED)
-    { /* No.  So allocate them with malloc.  We allocate the arrays
-	 for the start and end in one block.  */
+    { /* No.  So allocate them with malloc.  */
       regs->start = re_malloc (regoff_t, need_regs);
       regs->end = re_malloc (regoff_t, need_regs);
       if (BE (regs->start == NULL, 0) || BE (regs->end == NULL, 0))
@@ -467,10 +466,12 @@ re_copy_regs (regs, pmatch, nregs, regs_
 	 leave it alone.  */
       if (BE (need_regs > regs->num_regs, 0))
 	{
-	  regs->start = re_realloc (regs->start, regoff_t, need_regs);
-	  regs->end = re_realloc (regs->end, regoff_t, need_regs);
-	  if (BE (regs->start == NULL, 0) || BE (regs->end == NULL, 0))
+	  regoff_t *new_start = re_realloc (regs->start, regoff_t, need_regs);
+	  regoff_t *new_end = re_realloc (regs->end, regoff_t, need_regs);
+	  if (BE (new_start == NULL, 0) || BE (new_end == NULL, 0))
 	    return REGS_UNALLOCATED;
+	  regs->start = new_start;
+	  regs->end = new_end;
 	  regs->num_regs = need_regs;
 	}
     }
@@ -1133,7 +1134,7 @@ proceed_next_node (preg, nregs, regs, mc
       int ndest, dest_nodes[2];
       err = re_node_set_insert (eps_via_nodes, node);
       if (BE (err < 0, 0))
-	return -1;
+	return -2;
       /* Pick up valid destinations.  */
       for (ndest = 0, i = 0; i < dfa->edests[node].nelem; ++i)
 	{
@@ -1149,8 +1150,10 @@ proceed_next_node (preg, nregs, regs, mc
       /* In order to avoid infinite loop like "(a*)*".  */
       if (re_node_set_contains (eps_via_nodes, dest_nodes[0]))
 	return dest_nodes[1];
-      if (fs != NULL)
-	push_fail_stack (fs, *pidx, dest_nodes, nregs, regs, eps_via_nodes);
+      if (fs != NULL
+	  && push_fail_stack (fs, *pidx, dest_nodes, nregs, regs,
+			      eps_via_nodes))
+	return -2;
       return dest_nodes[0];
     }
   else
@@ -1220,11 +1223,11 @@ push_fail_stack (fs, str_idx, dests, nre
   if (fs->num == fs->alloc)
     {
       struct re_fail_stack_ent_t *new_array;
-      fs->alloc *= 2;
       new_array = realloc (fs->stack, (sizeof (struct re_fail_stack_ent_t)
-				       * fs->alloc));
+				       * fs->alloc * 2));
       if (new_array == NULL)
 	return REG_ESPACE;
+      fs->alloc *= 2;
       fs->stack = new_array;
     }
   fs->stack[num].idx = str_idx;
@@ -1246,7 +1249,7 @@ pop_fail_stack (fs, pidx, nregs, regs, e
 {
   int num = --fs->num;
   assert (num >= 0);
- *pidx = fs->stack[num].idx;
+  *pidx = fs->stack[num].idx;
   memcpy (regs, fs->stack[num].regs, sizeof (regmatch_t) * nregs);
   re_node_set_free (eps_via_nodes);
   re_free (fs->stack[num].regs);
@@ -1329,7 +1332,11 @@ set_regs (preg, mctx, nmatch, pmatch, fl
       if (BE (cur_node < 0, 0))
 	{
 	  if (cur_node == -2)
-	    return REG_ESPACE;
+	    {
+	      re_node_set_free (&eps_via_nodes);
+	      free_fail_stack_return (fs);
+	      return REG_ESPACE;
+	    }
 	  if (fs)
 	    cur_node = pop_fail_stack (fs, &idx, nmatch, pmatch,
 				       &eps_via_nodes);
@@ -2911,21 +2918,12 @@ check_arrival_add_next_nodes (preg, dfa,
 		      re_node_set_free (&union_set);
 		      return err;
 		    }
-		  err = re_node_set_insert (&union_set, next_node);
-		  if (BE (err < 0, 0))
-		    {
-		      re_node_set_free (&union_set);
-		      return REG_ESPACE;
-		    }
 		}
-	      else
+	      err = re_node_set_insert (&union_set, next_node);
+	      if (BE (err < 0, 0))
 		{
-		  err = re_node_set_insert (&union_set, next_node);
-		  if (BE (err < 0, 0))
-		    {
-		      re_node_set_free (&union_set);
-		      return REG_ESPACE;
-		    }
+		  re_node_set_free (&union_set);
+		  return REG_ESPACE;
 		}
 	      mctx->state_log[next_idx] = re_acquire_state (&err, dfa,
 							    &union_set);

	Jakub

Re: [PATCH] Small regexec.c fixes

[Ulrich Drepper]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Jakub Jelinek wrote:

> 	* posix/regexec.c (re_copy_regs): Revert comment change.
> 	Avoid memory leak if realloc fails.
> 	(proceed_next_node): Return -2 if re_node_set_insert fails.
> 	Return -2 if push_fail_stack fails.
> 	(push_fail_stack): Change fs->alloc only after successful
> 	realloc.
> 	(pop_fail_stack): Formatting.
> 	(set_regs): If proceed_next_node returns -2, free eps_via_nodes
> 	and fs.
> 	(check_arrival_add_next_nodes): Merge identical statements
> 	from if branches.

I've applied the patch but in general we should avoid adding much code
to free memory in case allocation fails.  Maybe to free some huge blocks
but not more.  If the app runs out of memory it'll very soon terminate.
 No need to bloat the code to help doing this.

- -- 
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.3 (GNU/Linux)

iD8DBQE/8Gq92ijCOnn/RHQRAmTxAKCOhphWPBMl5Un4958h3wkO/7/FeACgrHHg
Boh1oDn70uJW50RPVhJD2k4=
=RcIG
-----END PGP SIGNATURE-----

Re: [PATCH] Small regexec.c fixes

[Roland McGrath]

> I've applied the patch but in general we should avoid adding much code
> to free memory in case allocation fails.  Maybe to free some huge blocks
> but not more.  If the app runs out of memory it'll very soon terminate.
>  No need to bloat the code to help doing this.

I disagree completely.  It is important that library code never leak.  It
very well might be used in server situations where allocations can fail at
maximum load, or with unreasonable client input, but then enough more
memory gets freed by cleaning up resources associated with clients as they
die, that the server need never die.