bugmake - Bugs: bug #65588, A buffer overrun in handling of...

 
 

bug #65588: A buffer overrun in handling of .SHELLFLAGS.

Submitter:  Dmitry Goncharov <dgoncharov>
Submitted:  Sat 13 Apr 2024 02:13:34 PM UTC
Votes: 10
 
Severity:  3 - Normal Item Group:  Bug
Status:  None Privacy:  Public
Assigned to:  None Open/Closed:  Open
Component Version:  SCM Operating System:  None
Fixed Release:  None Triage Status:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 01 Oct 2024 12:31:12 AM UTC, comment #6: 

comment #5:

> I am experiencing a situation with GNU Make 4.4.1 that is I expect will be resolved with the changes under consideration.


i expect the same.

> Can you possibly advise me how I can test it in my environment.  I will build make if needed.


It is possible to apply the patches attached here. However, job.c has changed since the patches were crafted and you'd need to resolve conflicts.
It will be easier for you to build make from this branch https://github.com/dgoncharov/make/tree/sv65588_shellflags.


> I have a workaround, but would like to have this fix.


Hopefully, Paul can include this fix in the next release.

Dmitry Goncharov <dgoncharov>
Mon 30 Sep 2024 07:27:45 PM UTC, comment #5: 

I am experiencing a situation with GNU Make 4.4.1 that is I expect will be resolved with the changes under consideration.

Can you possibly advise me how I can test it in my environment. I will build make if needed.

In the following 4 one-liners, the first 3 work as expected but only the 4th segfaults, resulting from the combination of using both .ONESHELL and special characters in .SHELLFLAGS.


$ make -E $'SHELL=echo\n.SHELLFLAGS=hello\nall:\n\tworld\n\tsecond world' -s
hello world
hello second world
$ make -E $'.ONESHELL:\nSHELL=echo\n.SHELLFLAGS=hello\nall:\n\tworld\n\tsecond world' -s
hello world
second world
$ make -E $'SHELL=echo\n.SHELLFLAGS="hello"\nall:\n\tworld\n\tsecond world' -s
hello world
hello second world
$ make -E $'.ONESHELL:\nSHELL=echo\n.SHELLFLAGS="hello"\nall:\n\tworld\n\tsecond world' -s
/bin/sh -c "hello" world
second world
Segmentation fault (core dumped)


FWIW - I don't use `echo` as my SHELL.  My example is meant to be minimal and illustrative.  I encountered this while trying to use


SHELL=duckdb
.SHELLFLAGS= -c "attach 'mwftk.ddb'" -c


I have a workaround, but would like to have this fix.

malcolm cook <malcook>
Sat 27 Apr 2024 03:34:28 PM UTC, comment #4: 

Indeed. Thank you. job.c changed.

Paul, do you think we can switch to git branches? i can maintain a branch until merge. i suspect, working with a branch can be easier, than if i attach different versions of a patch.

Dmitry Goncharov <dgoncharov>
Fri 26 Apr 2024 08:50:50 PM UTC, comment #3: 

Your first patch (sv65588_part1.diff) doesn't apply currently. The rejected snippet is:

--- src/job.c
+++ src/job.c
@@ -2839,7 +2841,8 @@ construct_command_argv_internal (char *line, char **restp, const char *shell,
     return 0;

   if (shellflags == 0)
-    shellflags = posix_pedantic && NONE_SET (flags, COMMANDS_NOERROR) ? "-ec" : "-c";
+    shellflags = defsh_flags;
+  assert (shellflags);

   /* See if it is safe to parse commands internally.  */
   if (shell == 0)


The rest of the patches apply fine and fix the issue for me.

Andrei Horodniceanu <horo>
Sun 21 Apr 2024 09:15:53 PM UTC, comment #2: 

sv65588_part1.diff and sv65588_part2.diff fix the buffer overflow.
Make (with sv65588_part1.diff and sv65588_part2.diff applied) behaves
differently in oneshell mode compared to when .ONESHELL is not specified.
Specifically when .ONESHELL is not specified and shellflags carries an
unterminated quote, make switches to the slow mode to have the shell deal with the
quote.


$ ls
makefile
$ cat makefile
.SHELLFLAGS:=hello'
all:;:
$
$
$ make
:
/bin/sh: 1: Syntax error: Unterminated quoted string
make: *** [makefile:2: all] Error 2


When .ONESHELL is specified, make (with sv65588_part1.diff and
sv65588_part2.diff applied) proceeds to run the job regardless of the
unterminated quote.


$ cat makefile
.ONESHELL:
.SHELLFLAGS:=hello'
all:;:
$ make
:
/bin/sh: 0: cannot open hello': No such file
make: *** [makefile:3: all] Error 2



sv65588_part3.diff modifies construct_command_argv_internal to have oneshell
mode handle an unterminated quote the same as when .ONESHELL is not specified.

Tested on linux, sunos and aix, all 64 and 32 bits.

Dmitry Goncharov <dgoncharov>
Sat 13 Apr 2024 02:30:23 PM UTC, comment #1: 

A buffer overflow occurs in oneshell mode when shellflags contains characters
special to shell.


$ cat makefile
.ONESHELL:
.SHELLFLAGS:=hello'
all:;:
$ make
Aborted (core dumped)


In oneshell mode construct_command_argv_internal wants to store in argv[0]
the value of shell, followed by shellflags, followed by the command line.
construct_command_argv_internal preallocates memory with

nextp = new_argv[0] = xmalloc (shell_len + sflags_len + line_len + 3);

construct_command_argv_internal is then used recursively to construct argv from
shellflags.  If shellflags contains characters special to shell, then this
recursive call does the so-called slow mode and constructs argv of "/bin/sh",
"-c", and the value of shellflags.
One problem is that the top construct_command_argv_internal never allocated
room for the "/bin/sh" and "-c".

Aside from the overflow, it is incorrect to prepend "/bin/sh" and "-c" to
shellflags. Preallocating more room in top construct_command_argv_internal can
solve the overflow, but the resultant command line stays incorrect, because
shellflags were not supposed to carry "/bin/sh" and "-c".

This fix parses shellflags without help from construct_command_argv_internal
and lets shellflags carry quoted tokens in oneshell mode, even when special
characters are present.

The fix is split to 2 parts to simplify review.
Part1 is refactoring which allows to avoid multiple redundant if checks.
Part2 is the actual fix and is supposed to be applied after part1.

Tested on linux-x86, sunos-sparc and aix-powerpc, all 64 and 32 bits.

Dmitry Goncharov <dgoncharov>
Sat 13 Apr 2024 02:13:34 PM UTC, original submission:  

.

Dmitry Goncharov <dgoncharov>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #55953:  sv65588_part3.diff added by dgoncharov (5KiB - text/x-patch)
file #55942:  sv65588_part2.diff added by dgoncharov (104KiB - text/x-patch)
file #55941:  sv65588_part1.diff added by dgoncharov (5KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by malcook (Posted a comment)
  • -email is unavailable- added by malcook (Voted in favor of this item)
  • -email is unavailable- added by horo (Posted a comment)
  • -email is unavailable- added by dgoncharov (Submitted the item)
  •  

    There are 10 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2024-09-30 malcook Carbon-Copy- Added malcook
    2024-04-21 dgoncharov Attached File- Added sv65588_part3.diff, #55953
    2024-04-13 dgoncharov Attached File- Added sv65588_part1.diff, #55941
        Attached File- Added sv65588_part2.diff, #55942

    Back to the top

    Powered by Savane 3.14-fc07.
    Corresponding source code