diff mbox series

[2/3] posix: Remove alloca usage on regex build_trtable

Message ID 20210106181707.1738066-2-adhemerval.zanella@linaro.org
State Superseded
Headers
Series [1/3] posix: Remove alloca usage on regex set_regs |

Commit Message

Adhemerval Zanella Netto Jan. 6, 2021, 6:17 p.m. UTC 
  __libc_use_alloca/alloca is replaced with malloc regardless.

Checked on x86_64-linux-gnu.
---
 posix/regexec.c | 50 +++++++++++++++----------------------------------
 1 file changed, 15 insertions(+), 35 deletions(-)

Comments

Paul Eggert Jan. 8, 2021, 10:30 p.m. UTC | #1 
On 1/6/21 10:17 AM, Adhemerval Zanella wrote:
> __libc_use_alloca/alloca is replaced with malloc regardless.

These allocations are so small that they should be put on the stack 
instead of using malloc. I did that in Gnulib by installing the attached 
patch. The idea is that the resulting regexec.c file should be copyable 
unchanged into glibc.

 From a Gnulib point of view this code uses a 20 KiB frame (on a 64-bit 
host) which goes past the usual 4032-byte limit for stack frames, but I 
think we can stretch the point here.  In glibc the limit is 64 KiB so 
there's no problem.
Adhemerval Zanella Netto Jan. 11, 2021, 12:31 p.m. UTC | #2 
On 08/01/2021 19:30, Paul Eggert wrote:
> On 1/6/21 10:17 AM, Adhemerval Zanella wrote:
>> __libc_use_alloca/alloca is replaced with malloc regardless.
> 
> These allocations are so small that they should be put on the stack instead of using malloc. I did that in Gnulib by installing the attached patch. The idea is that the resulting regexec.c file should be copyable unchanged into glibc.
> 
> From a Gnulib point of view this code uses a 20 KiB frame (on a 64-bit host) which goes past the usual 4032-byte limit for stack frames, but I think we can stretch the point here.  In glibc the limit is 64 KiB so there's no problem.

Right, I think we can maybe use scratch_buffer for these or even a dynamic array
with a more proper initial size as an improvement.
diff mbox series

Patch

diff --git a/posix/regexec.c b/posix/regexec.c
index 5e22f90842..a8e9a9cd01 100644
--- a/posix/regexec.c
+++ b/posix/regexec.c
@@ -3259,8 +3259,6 @@  build_trtable (const re_dfa_t *dfa, re_dfastate_t *state)
   int ch;
   bool need_word_trtable = false;
   bitset_word_t elem, mask;
-  bool dests_node_malloced = false;
-  bool dest_states_malloced = false;
   Idx ndests; /* Number of the destination states from 'state'.  */
   re_dfastate_t **trtable;
   re_dfastate_t **dest_states = NULL, **dest_states_word, **dest_states_nl;
@@ -3278,15 +3276,9 @@  build_trtable (const re_dfa_t *dfa, re_dfastate_t *state)
      from 'state'.  'dests_node[i]' represents the nodes which i-th
      destination state contains, and 'dests_ch[i]' represents the
      characters which i-th destination state accepts.  */
-  if (__libc_use_alloca (sizeof (struct dests_alloc)))
-    dests_alloc = (struct dests_alloc *) alloca (sizeof (struct dests_alloc));
-  else
-    {
-      dests_alloc = re_malloc (struct dests_alloc, 1);
-      if (__glibc_unlikely (dests_alloc == NULL))
-	return false;
-      dests_node_malloced = true;
-    }
+  dests_alloc = re_malloc (struct dests_alloc, 1);
+  if (__glibc_unlikely (dests_alloc == NULL))
+    return false;
   dests_node = dests_alloc->dests_node;
   dests_ch = dests_alloc->dests_ch;
 
@@ -3298,8 +3290,7 @@  build_trtable (const re_dfa_t *dfa, re_dfastate_t *state)
   ndests = group_nodes_into_DFAstates (dfa, state, dests_node, dests_ch);
   if (__glibc_unlikely (ndests <= 0))
     {
-      if (dests_node_malloced)
-	re_free (dests_alloc);
+      re_free (dests_alloc);
       /* Return false in case of an error, true otherwise.  */
       if (ndests == 0)
 	{
@@ -3323,26 +3314,17 @@  build_trtable (const re_dfa_t *dfa, re_dfastate_t *state)
   if (__glibc_unlikely (ndests_max < ndests))
     goto out_free;
 
-  if (__libc_use_alloca ((sizeof (re_node_set) + sizeof (bitset_t)) * SBC_MAX
-			 + ndests * 3 * sizeof (re_dfastate_t *)))
-    dest_states = (re_dfastate_t **)
-      alloca (ndests * 3 * sizeof (re_dfastate_t *));
-  else
+  dest_states =
+    re_malloc (re_dfastate_t *, ndests * 3 * sizeof (re_dfastate_t *));
+  if (__glibc_unlikely (dest_states == NULL))
     {
-      dest_states = re_malloc (re_dfastate_t *, ndests * 3);
-      if (__glibc_unlikely (dest_states == NULL))
-	{
 out_free:
-	  if (dest_states_malloced)
-	    re_free (dest_states);
-	  re_node_set_free (&follows);
-	  for (i = 0; i < ndests; ++i)
-	    re_node_set_free (dests_node + i);
-	  if (dests_node_malloced)
-	    re_free (dests_alloc);
-	  return false;
-	}
-      dest_states_malloced = true;
+      re_free (dest_states);
+      re_node_set_free (&follows);
+      for (i = 0; i < ndests; ++i)
+	re_node_set_free (dests_node + i);
+      re_free (dests_alloc);
+      return false;
     }
   dest_states_word = dest_states + ndests;
   dest_states_nl = dest_states_word + ndests;
@@ -3470,15 +3452,13 @@  out_free:
 	  }
     }
 
-  if (dest_states_malloced)
-    re_free (dest_states);
+  re_free (dest_states);
 
   re_node_set_free (&follows);
   for (i = 0; i < ndests; ++i)
     re_node_set_free (dests_node + i);
 
-  if (dests_node_malloced)
-    re_free (dests_alloc);
+  re_free (dests_alloc);
 
   return true;
 }