Re: Segmentation Fault on Exported Resursively Expanded Variable

[Henrik Carlqvist]

On Tue, 16 Jan 2024 20:53:19 +0200
Eli Zaretskii  wrote:
> From: Henrik Carlqvist 
> > On Tue, 16 Jan 2024 06:59:30 +
> > MIAOW Miao  wrote:
> > > if ((*ep)[nl] == '=' && strncmp (*ep, v->name, nl) == 0)
> > 
> > Looking at that line, the rather obvious fix would be to change it to:
> > 
> > if (strncmp (*ep, v->name, nl) == 0 && (*ep)[nl] == '=')
> > 
> > That way, *ep can be no shorter than having \0 at position nl and
> > accessing that position should not cause any segfault.
> 
> But it's less efficient when the (*ep)[nl] == '=' test fails.

Yes, that is true, but to avoid a possible segfault it is necessary to somehow
check that (*ep)[nl] is a valid address. The current fix at
https://savannah.gnu.org/bugs/index.php?65172 also works fine, but also that
fix might be even less efficient as strlen will read every char up to and
including \0 in *ep.

regards Henrik

Re: Segmentation Fault on Exported Resursively Expanded Variable

[Dmitry Goncharov]

On Mon, Jan 15, 2024 at 9:02 AM MIAOW Miao  wrote:
> Here is a Makefile that can reproduce the segmentation fault:
>
> THIS_LONG_VARIABLE_NAME_PREDUCE_THE_ERROR= $(shell echo hello)
> export THIS_LONG_VARIABLE_NAME_PREDUCE_THE_ERROR
>
> all: ; echo "abc"


Thank you for your report.
A added a fix here https://savannah.gnu.org/bugs/index.php?65172.

regards, Dmitry

[bug #65172] Fix a buffer overrun on a variable with a long name.

[Dmitry Goncharov]

Follow-up Comment #1, bug#65172 (group make):

A user reported a buffer overflow on a variable with a long name.


Here is a fix.

[SV 65172] Fix a buffer overrun on a variable with a long name.

* src/expand.c (recursively_expand_for_file): Fix a buffer overrun.
* tests/scripts/functions/shell: Add a test.

diff --git a/src/expand.c b/src/expand.c
index fe09c9c3..283a3d47 100644
--- a/src/expand.c
+++ b/src/expand.c
@@ -164,9 +164,11 @@ recursively_expand_for_file (struct variable *v, struct
file *file)
   /* We could create a hash for the original environment for speed, but
a
  reasonably written makefile shouldn't hit this situation...  */
   for (ep = environ; *ep != 0; ++ep)
-if ((*ep)[nl] == '=' && strncmp (*ep, v->name, nl) == 0)
-  return xstrdup ((*ep) + nl + 1);
-
+{
+  size_t len = strlen (*ep);
+  if (len >= nl && (*ep)[nl] == '=' && memcmp (*ep, v->name, nl) ==
0)
+return xstrdup ((*ep) + nl + 1);
+}
   /* If there's nothing in the parent environment, use the empty string.
  This isn't quite correct since the variable should not exist at
all,
  but getting that to work would be involved. */




Here is a test.

[SV 65172] Fix a buffer overrun on a variable with a long name.

* src/expand.c (recursively_expand_for_file): Fix a buffer overrun.
* tests/scripts/functions/shell: Add a test.

diff --git a/tests/scripts/functions/shell b/tests/scripts/functions/shell
index e5c346cc..b9b9ee32 100644
--- a/tests/scripts/functions/shell
+++ b/tests/scripts/functions/shell
@@ -213,4 +213,15 @@ endif
   '--no-print-directory -j2', ": 2\n: 1");
 }

+if ($port_type eq 'UNIX') {
+# sv 65172.
+# Buffer overrun in recursively_expand_for_file on a variable with a
long
+# name.
+my $v = "a1234567890" x 4 x 1000;
+run_make_test("
+export $v=\$(shell echo hello)
+all:; \@echo \$\$$v
+", '', "hello\n");
+}
+
 1;



The original mail is here
https://lists.gnu.org/archive/html/bug-make/2024-01/msg00044.html


___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #65172] Fix a buffer overrun on a variable with a long name.

[Dmitry Goncharov]

URL:
  

 Summary: Fix a buffer overrun on a variable with a long name.
   Group: make
   Submitter: dgoncharov
   Submitted: Tue 16 Jan 2024 10:43:18 PM UTC
Severity: 3 - Normal
  Item Group: Bug
  Status: None
 Privacy: Public
 Assigned to: None
 Open/Closed: Open
 Discussion Lock: Any
   Component Version: SCM
Operating System: Any
   Fixed Release: None
   Triage Status: None


___

Follow-up Comments:


---
Date: Tue 16 Jan 2024 10:43:18 PM UTC By: Dmitry Goncharov 
.







___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

Re: Segmentation Fault on Exported Resursively Expanded Variable

[Eli Zaretskii]

> Date: Tue, 16 Jan 2024 19:21:04 +0100
> From: Henrik Carlqvist 
> Cc: psm...@gnu.org, bug-make@gnu.org
> 
> On Tue, 16 Jan 2024 06:59:30 +
> MIAOW Miao  wrote:
> > if ((*ep)[nl] == '=' && strncmp (*ep, v->name, nl) == 0)
> 
> Looking at that line, the rather obvious fix would be to change it to:
> 
> if (strncmp (*ep, v->name, nl) == 0 && (*ep)[nl] == '=')
> 
> That way, *ep can be no shorter than having \0 at position nl and accessing
> that position should not cause any segfault.

But it's less efficient when the (*ep)[nl] == '=' test fails.

Re: Segmentation Fault on Exported Resursively Expanded Variable

[Henrik Carlqvist]

On Tue, 16 Jan 2024 06:59:30 +
MIAOW Miao  wrote:
> if ((*ep)[nl] == '=' && strncmp (*ep, v->name, nl) == 0)

Looking at that line, the rather obvious fix would be to change it to:

if (strncmp (*ep, v->name, nl) == 0 && (*ep)[nl] == '=')

That way, *ep can be no shorter than having \0 at position nl and accessing
that position should not cause any segfault.

regards Henrik

Re: Re: Segmentation Fault on Exported Resursively Expanded Variable

[Paul Smith]

On Tue, 2024-01-16 at 06:59 +, MIAOW Miao wrote:
> (gdb) bt
> #0  recursively_expand_for_file (v=v@entry=0xad5740,
> file=file@entry=0x0) at src/expand.c:119
> ...
> #20 0x0041acec in main (argc=, argv= out>, envp=) at src/main.c:2918

This is not as helpful because you've elided the entire stack trace. 
We can infer it from Henrik's stack trace but he is working with
modified code so getting it from a vanilla copy would be better.

>From Henrik's investigation and the fact that I cannot reproduce it,
I'm pretty sure it's related to your environment; almost certainly to
the environment variables in your environment when you invoke make.

Dmitry says he can't reproduce it in the latest code from Git, but
since I can't repro it with either 4.4 or 4.4.1 I'm not sure that's
determinative.  If Dmitry CAN repro it reliably with vanilla 4.4 or
4.4.1 but not with the latest code then we can say it's been resolved.