Re: Segmentation Fault on Exported Resursively Expanded Variable
On Sun, 21 Jan 2024 14:45:00 -0500 Dmitry Goncharov wrote: > i bet, the purpose of having (*ep)[nl] == '=' check before strncmp was > to relieve make from running strncmp unless the variables have the > same length. Yes, that is also my guess. Unfortunately, it could give a segfault if (*ep)[nl] is not a valid memory location. > For every char, strncmp does two checks, n and the character. strlen > does only one check. > Without doing any measurements, i expect, strlen do better than > strncmp when strlen (*ep) is shorter than nl. > On the other hand, when v->name is half the length of *ep, we'd prefer > strncmp. I haven't done any measurements either, but my guess is that strlen is faster for the cases when the strlen(*ep) is shorter than about double the position of the first different characters in *ep and v->name. So strncmp might be faster if the strings differ in the beginning as strncmp probably will abort at the first difference but strlen will not abort until the end of the string. But on the other hand, strlen will be at least twice as fast for every position as it only cares about a single string and does not care about any max length. regards Henrik
Re: Segmentation Fault on Exported Resursively Expanded Variable
On Tue, Jan 16, 2024 at 1:21 PM Henrik Carlqvist wrote: > > 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] == '=') i bet, the purpose of having (*ep)[nl] == '=' check before strncmp was to relieve make from running strncmp unless the variables have the same length. Similarly, the fix attached to the savannah report size_t len = strlen (*ep); if (len >= nl && (*ep)[nl] == '=' && memcmp (*ep, v->name, nl) == 0) does the length check before memcmp for the same purpose, to relieve make from running memcmp, unless needed. For every char, strncmp does two checks, n and the character. strlen does only one check. Without doing any measurements, i expect, strlen do better than strncmp when strlen (*ep) is shorter than nl. On the other hand, when v->name is half the length of *ep, we'd prefer strncmp. regards, Dmitry
Re: Segmentation Fault on Exported Resursively Expanded Variable
MIAOW Miao wrote: if ((*ep)[nl] == '=' && strncmp (*ep, v->name, nl) == 0) Henrik Carlqvist (Tue, 16 Jan 2024 06:59:30 +) wrote: >>> 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. On Tue, 16 Jan 2024 20:53:19 +0200 Eli Zaretskii wrote: >> But it's less efficient when the (*ep)[nl] == '=' test fails. Henrik Carlqvist (17 January 2024 07:24) wrote: > 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. or, to look at it another way, you can't legally access (*ep)[nl] until you've looked at every (*ep)[i] for 0 <= i < nl, since if any of those is '\0' then the memory segment you're allowed to access might end before (*ep)[nl], making it an invalid memory access. Henrik's proposed fix Does The Right Thing and is cleaner than the current proposed fix, Eddy.
Re: Segmentation Fault on Exported Resursively Expanded Variable
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
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
Re: Segmentation Fault on Exported Resursively Expanded Variable
> 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
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
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.
Re: Segmentation Fault on Exported Resursively Expanded Variable
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" > > Commands log: > > user@fedora:~$ make > echo "abc" > Segmentation fault (core dumped) Does not reproduce for me with make from master. regards, Dmitry
Re: Segmentation Fault on Exported Resursively Expanded Variable
I was not able to reproduce it with Make 4.1 or Make 4.3. I don't have the original Make 4.4.1, but I had a binary of my fork 4.4.1h2 from https://github.com/henca/Henriks-make and got a segfault in that one which probably means that I have been able to reproduce the report from Miao. Opening a core dump i ddd it seems as if the segfault was at: src/expand.c:119 called from variable.c:1143 called from function.c:1878 called from function.c:2693 called from expand.c:282 called from expand.c:441 called from expand.c:448 called from expand.c:590 ... Or copy paste from the debugger: Core was generated by `./make'. Program terminated with signal SIGSEGV, Segmentation fault. #0 recursively_expand_for_file (v=v@entry=0x13e5990, file=file@entry=0x0) at src/expand.c:119 (gdb) bt #0 recursively_expand_for_file (v=v@entry=0x13e5990, file=file@entry=0x0) at #src/expand.c:119 1 0x00429bad in target_environment #(file=file@entry=0x0, recursive=recursive@entry=0) at src/variable.c:1143 2 #0x004133dc in func_shell_base (o=0x13f2680 "", argv=, #trim_newlines=1) at src/function.c:1878 3 0x00413923 in #handle_function (op=op@entry=0x7ffd1edc1428, #stringp=stringp@entry=0x7ffd1edc1420) at src/function.c:2693 4 #0x0040d3c2 in variable_expand_string (line=, #line@entry=0x0, string=, #length=length@entry=18446744073709551615) at src/expand.c:282 5 #0x0040db5e in variable_expand (line=) at #src/expand.c:441 6 variable_expand_for_file (file=0x0, line=) #at src/expand.c:488 7 allocated_variable_expand_for_file (file=0x0, #line=) at src/expand.c:590 8 recursively_expand_for_file #(v=v@entry=0x13e5990, file=file@entry=0x13e5a30) at src/expand.c:164 9 #0x00429bad in target_environment (file=, #recursive=) at src/variable.c:1143 10 0x004188d2 in #start_job_command (child=child@entry=0x13f25c0) at src/job.c:1431 11 #0x004192d4 in start_waiting_job (c=c@entry=0x13f25c0) at #src/job.c:1646 12 0x00419cf2 in new_job (file=0x13e5a30) at #src/job.c:1960 13 0x00425a95 in remake_file (file=0x13e5a30) at #src/remake.c:1313 14 update_file_1 (depth=, file=) at src/remake.c:905 15 update_file (file=file@entry=0x13e5a30, #depth=) at src/remake.c:367 16 0x004263e1 in #update_goal_chain (goaldeps=) at src/remake.c:184 17 #0x00409045 in main (argc=, argv=, #envp=) at src/main.c:2951 (gdb) Looking at the variables in ddd I see that *ep is a string "VDPAU_LOG=0" probably taken from my environment. The variable nl is 41 which is way beyond that string and probably causing the segfault. The nl variable comes from strlen(v->name) where v->name is "THIS_LONG_VARIABLE_NAME_PRODUCE_THE_ERROR". So it seems to me that such a long variable name can cause a segfault if an environment variable is short enough. I realize a report like this would be more valuable if I had the original gnu make. On my fork I have modified main.c and job.c (and posixos.c) making any line numbers from those files not trustworthy. However, I still hope that my analysis will be useful. regards Henrik On Mon, 15 Jan 2024 14:37:31 -0500 Paul Smith wrote: > On Mon, 2024-01-15 at 11:21 +, MIAOW Miao wrote: > > I found name of exported resursively expanded variable with $(shell > > ...) cannot be too long in gnu make version >= 4.4, otherwise a > > segmentation fault is triggled. I'm not sure if variable-name-too- > > long is a bug. However, make is > > supposed to tell me what's going wrong. > > > > Here is a Makefile that can reproduce the segmentation fault: > > > THIS_LONG_VARIABLE_NAME_PREDUCE_THE_ERROR= $(shell echo hello) > > > exportTHIS_LONG_VARIABLE_NAME_PREDUCE_THE_ERROR > > > > > > all: ; echo "abc" > > I was not able to reproduce this problem, either with my own build of > GNU Make 4.4.1 or with a binary extracted from the RPM from the link > you provided. I tried running under valgrind and with a binary > compiled with ASAN, with and withoug debugging enabled, ran the test > 1000 times. I also tried GNU Make 4.4. > > If you can generate a coredump please examine it with GDB and send > along at least the backtrace. > > -- > Paul D. Smith Find some GNU make tips at: > https://www.gnu.org http://make.mad-scientist.net > "Please remain calm...I may be mad, but I am a professional." --Mad > Scientist >
Re: Segmentation Fault on Exported Resursively Expanded Variable
On Mon, 2024-01-15 at 11:21 +, MIAOW Miao wrote: > I found name of exported resursively expanded variable with $(shell > ...) cannot be too long in gnu make version >= 4.4, otherwise a > segmentation fault is triggled. I'm not sure if variable-name-too- > long is a bug. However, make is > supposed to tell me what's going wrong. > > Here is a Makefile that can reproduce the segmentation fault: > > THIS_LONG_VARIABLE_NAME_PREDUCE_THE_ERROR= $(shell echo hello) > > exportTHIS_LONG_VARIABLE_NAME_PREDUCE_THE_ERROR > > > > all: ; echo "abc" I was not able to reproduce this problem, either with my own build of GNU Make 4.4.1 or with a binary extracted from the RPM from the link you provided. I tried running under valgrind and with a binary compiled with ASAN, with and withoug debugging enabled, ran the test 1000 times. I also tried GNU Make 4.4. If you can generate a coredump please examine it with GDB and send along at least the backtrace. -- Paul D. Smith Find some GNU make tips at: https://www.gnu.org http://make.mad-scientist.net "Please remain calm...I may be mad, but I am a professional." --Mad Scientist
Segmentation Fault on Exported Resursively Expanded Variable
Hi, I found name of exported resursively expanded variable with $(shell ...) cannot be too long in gnu make version >= 4.4, otherwise a segmentation fault is triggled. I'm not sure if variable-name-too-long is a bug. However, make is supposed to tell me what's going wrong. 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" Commands log: user@fedora:~$ make echo "abc" Segmentation fault (core dumped) user@fedora:~$ make --version GNU Make 4.4.1 Built for x86_64-redhat-linux-gnu ... user@fedora:~$ make --help ... This program built for x86_64-redhat-linux-gnu Report bugs to The error can be reproduced on fedora 39 with the following 2 official builds of make by fedora project: * make-4.4-1.fc38: https://kojipkgs.fedoraproject.org//packages/make/4.4/1.fc38/x86_64/make-4.4-1.fc38.x86_64.rpm * make-4.4.1-2.fc39: https://kojipkgs.fedoraproject.org//packages/make/4.4.1/2.fc39/x86_64/make-4.4.1-2.fc39.x86_64.rpm There is NO error with gnu make 4.3, tested both on Ubuntu 23.04 (make 4.3-4.1build1) and fedora39 (make-4.3-1.fc33): * make 4.3-4.1build1: http://mirrors.kernel.org/ubuntu/pool/main/m/make-dfsg/make_4.3-4.1build1_amd64.deb * make-4.3-1.fc33: https://kojipkgs.fedoraproject.org//packages/make/4.3/1.fc33/x86_64/make-4.3-1.fc33.x86_64.rpm I think it may be caused by the new feature "shell-export". As is the "NEWS" file of Version 4.4 (31 Oct 2022): * WARNING: Backward-incompatibility! Previously makefile variables marked as export were not exported to command started by the $(shell ...) function. Now, all exported variables are exported to $(shell ...). If this leads to recursion during expansion, the for backward-compatibility the value from the original environment is used. To detect this change search for 'shell-export' in the .FEATURES variable. Best regards, Yiren Guo Makefile Description: Makefile
