[PATCH 0/2] Fix resource leaks in gnulib

[PATCH 0/2] Fix resource leaks in gnulib

[Alec Brown via Grub-devel]

Coverity found a couple resource leaks in gnulib code that the GRUB is using.
These fixes have been made in the latest version of gnulib and I've backported
these changes to maintain consistency.

This patch set fixes the following CIDs:
CID: 473869
CID: 473887
CID: 473888

Alec Brown (2):
      gnulib/regcomp: Fix resource leak
      gnulib/regexec: Fix resource leak

 bootstrap.conf                                               |   7 ++-
 conf/Makefile.extra-dist                                     |   2 +
 grub-core/lib/gnulib-patches/fix-regcomp-resource-leak.patch | 110 ++++++++++++++++++++++++++++++++++++++++++++++++
 grub-core/lib/gnulib-patches/fix-regexec-resource-leak.patch |  11 +++++
 4 files changed, 129 insertions(+), 1 deletion(-)



_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH 1/2] gnulib/regcomp: Fix resource leak

[Alec Brown via Grub-devel]

In the functions create_initial_state() and calc_eclosure_iter(), memory is
allocated for the elems member of a re_node_set structure but that memory
isn't freed on error. Before returning an error, a call to re_node_set_free()
should be made to prevent the resource leak.

This issue has been fixed in the latest version of gnulib and I've backported
this change to maintain consistency.

This issue was found by a Coverity Scan of GRUB2 under the following CIDs:
CID: 473869
CID: 473888

Signed-off-by: Alec Brown <alec.r.brown@oracle.com>
---
 bootstrap.conf                                |   5 +-
 conf/Makefile.extra-dist                      |   1 +
 .../fix-regcomp-resource-leak.patch           | 110 ++++++++++++++++++
 3 files changed, 115 insertions(+), 1 deletion(-)
 create mode 100644 grub-core/lib/gnulib-patches/fix-regcomp-resource-leak.patch

diff --git a/bootstrap.conf b/bootstrap.conf
index 7a7813d28..7a464a289 100644
--- a/bootstrap.conf
+++ b/bootstrap.conf
@@ -84,7 +84,10 @@ bootstrap_post_import_hook () {
   # Instead of patching our gnulib and therefore maintaining a fork, submit
   # changes to gnulib and update the hash above when they've merged.  Do not
   # add new patches here.
-  patch -d grub-core/lib/gnulib -p2 < grub-core/lib/gnulib-patches/fix-width.patch
+  for patchname in fix-width fix-regcomp-resource-leak; do
+    patch -d grub-core/lib/gnulib -p2 \
+       < "grub-core/lib/gnulib-patches/$patchname.patch"
+  done
 
   for patchname in \
       0001-Support-POTFILES-shell \
diff --git a/conf/Makefile.extra-dist b/conf/Makefile.extra-dist
index d9e2b8cc7..5bf3da429 100644
--- a/conf/Makefile.extra-dist
+++ b/conf/Makefile.extra-dist
@@ -30,6 +30,7 @@ EXTRA_DIST += grub-core/genemuinit.sh
 EXTRA_DIST += grub-core/genemuinitheader.sh
 
 EXTRA_DIST += grub-core/lib/gnulib-patches/fix-width.patch
+EXTRA_DIST += grub-core/lib/gnulib-patches/fix-regcomp-resource-leak.patch
 
 EXTRA_DIST += grub-core/lib/libgcrypt
 EXTRA_DIST += grub-core/lib/libgcrypt-grub/mpi/generic
diff --git a/grub-core/lib/gnulib-patches/fix-regcomp-resource-leak.patch b/grub-core/lib/gnulib-patches/fix-regcomp-resource-leak.patch
new file mode 100644
index 000000000..b2e900023
--- /dev/null
+++ b/grub-core/lib/gnulib-patches/fix-regcomp-resource-leak.patch
@@ -0,0 +1,110 @@
+--- a/lib/regcomp.c
++++ b/lib/regcomp.c
+@@ -1001,21 +1001,25 @@ create_initial_state (re_dfa_t *dfa)
+ 	    Idx dest_idx = dfa->edests[node_idx].elems[0];
+ 	    if (!re_node_set_contains (&init_nodes, dest_idx))
+ 	      {
+-		reg_errcode_t merge_err
++		err
+                   = re_node_set_merge (&init_nodes, dfa->eclosures + dest_idx);
+-		if (merge_err != REG_NOERROR)
+-		  return merge_err;
++		if (err != REG_NOERROR)
++		  break;
+ 		i = 0;
+ 	      }
+ 	  }
+       }
+ 
+   /* It must be the first time to invoke acquire_state.  */
+-  dfa->init_state = re_acquire_state_context (&err, dfa, &init_nodes, 0);
+-  /* We don't check ERR here, since the initial state must not be NULL.  */
++  dfa->init_state
++    = (err == REG_NOERROR
++       ? re_acquire_state_context (&err, dfa, &init_nodes, 0)
++       : NULL);
+   if (__glibc_unlikely (dfa->init_state == NULL))
+-    return err;
+-  if (dfa->init_state->has_constraint)
++    {
++      /* Don't check ERR here, as the initial state must not be null.  */
++    }
++  else if (dfa->init_state->has_constraint)
+     {
+       dfa->init_state_word = re_acquire_state_context (&err, dfa, &init_nodes,
+ 						       CONTEXT_WORD);
+@@ -1025,17 +1029,13 @@ create_initial_state (re_dfa_t *dfa)
+ 							 &init_nodes,
+ 							 CONTEXT_NEWLINE
+ 							 | CONTEXT_BEGBUF);
+-      if (__glibc_unlikely (dfa->init_state_word == NULL
+-			    || dfa->init_state_nl == NULL
+-			    || dfa->init_state_begbuf == NULL))
+-	return err;
+     }
+   else
+     dfa->init_state_word = dfa->init_state_nl
+       = dfa->init_state_begbuf = dfa->init_state;
+ 
+   re_node_set_free (&init_nodes);
+-  return REG_NOERROR;
++  return err;
+ }
+ 
+ /* If it is possible to do searching in single byte encoding instead of UTF-8
+@@ -1677,12 +1677,11 @@ calc_eclosure_iter (re_node_set *new_set, re_dfa_t *dfa, Idx node, bool root)
+     {
+       err = duplicate_node_closure (dfa, node, node, node,
+ 				    dfa->nodes[node].constraint);
+-      if (__glibc_unlikely (err != REG_NOERROR))
+-	return err;
+     }
+ 
+   /* Expand each epsilon destination nodes.  */
+-  if (IS_EPSILON_NODE(dfa->nodes[node].type))
++  if (__glibc_likely (err == REG_NOERROR)
++      && IS_EPSILON_NODE (dfa->nodes[node].type))
+     for (i = 0; i < dfa->edests[node].nelem; ++i)
+       {
+ 	re_node_set eclosure_elem;
+@@ -1700,14 +1699,14 @@ calc_eclosure_iter (re_node_set *new_set, re_dfa_t *dfa, Idx node, bool root)
+ 	  {
+ 	    err = calc_eclosure_iter (&eclosure_elem, dfa, edest, false);
+ 	    if (__glibc_unlikely (err != REG_NOERROR))
+-	      return err;
++	      break;
+ 	  }
+ 	else
+ 	  eclosure_elem = dfa->eclosures[edest];
+ 	/* Merge the epsilon closure of 'edest'.  */
+ 	err = re_node_set_merge (&eclosure, &eclosure_elem);
+ 	if (__glibc_unlikely (err != REG_NOERROR))
+-	  return err;
++	  break;
+ 	/* If the epsilon closure of 'edest' is incomplete,
+ 	   the epsilon closure of this node is also incomplete.  */
+ 	if (dfa->eclosures[edest].nelem == 0)
+@@ -1717,12 +1716,18 @@ calc_eclosure_iter (re_node_set *new_set, re_dfa_t *dfa, Idx node, bool root)
+ 	  }
+       }
+ 
+-  if (incomplete && !root)
+-    dfa->eclosures[node].nelem = 0;
++  if (err != REG_NOERROR)
++    re_node_set_free (&eclosure);
+   else
+-    dfa->eclosures[node] = eclosure;
+-  *new_set = eclosure;
+-  return REG_NOERROR;
++    {
++      if (incomplete && !root)
++	dfa->eclosures[node].nelem = 0;
++      else
++	dfa->eclosures[node] = eclosure;
++      *new_set = eclosure;
++    }
++
++  return err;
+ }
+ 
+ /* Functions for token which are used in the parser.  */
-- 
2.27.0


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH 2/2] gnulib/regexec: Fix resource leak

[Alec Brown via Grub-devel]

In the function merge_state_with_log(), memory is allocated for the variable
next_nodes when creating a union of the variables table_nodes and log_nodes.
However, if next_state->entrance_nodes is NULL, then table_nodes becomes NULL
and we still allocate memory to copy the content of log_nodes. This can cause a
resource leak since we only free the memory for next_nodes if table_nodes isn't
NULL. To prevent this, we need to check that next_state->entrance_nodes isn't
NULL before allocating memory for the union.

This issue has been fixed in the latest version of gnulib and I've backported
this change to maintain consistency.

This issue was found by a Coverity Scan of GRUB2 under the following CID:
CID: 473887

Signed-off-by: Alec Brown <alec.r.brown@oracle.com>
---
 bootstrap.conf                                        |  4 +++-
 conf/Makefile.extra-dist                              |  1 +
 .../gnulib-patches/fix-regexec-resource-leak.patch    | 11 +++++++++++
 3 files changed, 15 insertions(+), 1 deletion(-)
 create mode 100644 grub-core/lib/gnulib-patches/fix-regexec-resource-leak.patch

diff --git a/bootstrap.conf b/bootstrap.conf
index 7a464a289..7cd375ba9 100644
--- a/bootstrap.conf
+++ b/bootstrap.conf
@@ -84,7 +84,9 @@ bootstrap_post_import_hook () {
   # Instead of patching our gnulib and therefore maintaining a fork, submit
   # changes to gnulib and update the hash above when they've merged.  Do not
   # add new patches here.
-  for patchname in fix-width fix-regcomp-resource-leak; do
+  for patchname in fix-width \
+      fix-regcomp-resource-leak \
+      fix-regexec-resource-leak; do
     patch -d grub-core/lib/gnulib -p2 \
        < "grub-core/lib/gnulib-patches/$patchname.patch"
   done
diff --git a/conf/Makefile.extra-dist b/conf/Makefile.extra-dist
index 5bf3da429..07eee1956 100644
--- a/conf/Makefile.extra-dist
+++ b/conf/Makefile.extra-dist
@@ -31,6 +31,7 @@ EXTRA_DIST += grub-core/genemuinitheader.sh
 
 EXTRA_DIST += grub-core/lib/gnulib-patches/fix-width.patch
 EXTRA_DIST += grub-core/lib/gnulib-patches/fix-regcomp-resource-leak.patch
+EXTRA_DIST += grub-core/lib/gnulib-patches/fix-regexec-resource-leak.patch
 
 EXTRA_DIST += grub-core/lib/libgcrypt
 EXTRA_DIST += grub-core/lib/libgcrypt-grub/mpi/generic
diff --git a/grub-core/lib/gnulib-patches/fix-regexec-resource-leak.patch b/grub-core/lib/gnulib-patches/fix-regexec-resource-leak.patch
new file mode 100644
index 000000000..f490e05fb
--- /dev/null
+++ b/grub-core/lib/gnulib-patches/fix-regexec-resource-leak.patch
@@ -0,0 +1,11 @@
+--- a/lib/regexec.c
++++ b/lib/regexec.c
+@@ -2270,7 +2270,7 @@ merge_state_with_log (reg_errcode_t *err, re_match_context_t *mctx,
+ 	 these destinations and the results of the transition table.  */
+       pstate = mctx->state_log[cur_idx];
+       log_nodes = pstate->entrance_nodes;
+-      if (next_state != NULL)
++      if (next_state != NULL && next_state->entrance_nodes != NULL)
+ 	{
+ 	  table_nodes = next_state->entrance_nodes;
+ 	  *err = re_node_set_init_union (&next_nodes, table_nodes,
-- 
2.27.0


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 0/2] Fix resource leaks in gnulib

[Daniel Kiper via Grub-devel]

On Tue, Jun 10, 2025 at 03:19:43PM +0000, Alec Brown wrote:
> Coverity found a couple resource leaks in gnulib code that the GRUB is using.
> These fixes have been made in the latest version of gnulib and I've backported
> these changes to maintain consistency.
>
> This patch set fixes the following CIDs:
> CID: 473869
> CID: 473887
> CID: 473888
>
> Alec Brown (2):
>       gnulib/regcomp: Fix resource leak
>       gnulib/regexec: Fix resource leak

For both patches Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>...

Daniel

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 0/2] Fix resource leaks in gnulib

[Collin Funk]

Hi Alec,

Alec Brown via Grub-devel <grub-devel@gnu.org> writes:

> Coverity found a couple resource leaks in gnulib code that the GRUB is using.
> These fixes have been made in the latest version of gnulib and I've backported
> these changes to maintain consistency.

Thanks for fixing these in Gnulib.

There was a left shift overflow copied from Binutils that was fixed
there years ago. I sent a patch a while ago that got lost in the noise
[1]. Figured you may want to review it since it is simple.

Collin

[1] https://lists.nongnu.org/archive/html/grub-devel/2025-05/msg00027.html

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel