Fix issue with printvars executing giant shell command

PATCH New Trent Piepho 2 participants 6 messages Aug 17, 2018 – Sep 17, 2018 Kbuild

Share & tools

Trent Piepho

author

2018-08-17 23:15 UTC #0 lore raw

PATCH 1/1 Fix issue with printvars executing giant shell command

The underlying problem is that $(foreach V,1 2 3,) does not evaluate to an empty string. It evaluates to "? ", three empty strings separated by whitespace.

A construct of this format, with a giant list in the foreach, is part of the printvars command. This means that "@:$(foreach ....)", which is
intended to expand to a null command, in fact expands to "@: "
with a great deal of whitespace. Make chooses to execute this command with:

    execve("/bin/sh", ["/bin/sh", "-c", ":       "]

But with far more whitespace. So much that it can exceed shell command line length limits.

The solution here is to use $(strip $(foreach ...)), so the command expands to "@:", which make is smart enough to not even execute and wouldn't exceed any limits if it did.

In some cases, make will not invoke a command via "sh -c" and will exec it directly. In this case make does command line argument parsing and will swallow all the whitespace without overflowing a /bin/sh limit. It proved difficult to write a command that ensured make would do this.

Signed-off-by Trent Piepho <[email protected]>

---
 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Makefile +2 -2

index 0115c4dfcc..b396c56042 100644
@@ -988,13 +988,13 @@ $(BUILD_DIR)/.br2-external.in: $(BUILD_DIR)
 # displayed.
 .PHONY: printvars
 printvars:
-	@:$(foreach V, \
+	@:$(strip $(foreach V, \
 		$(sort $(if $(VARS),$(filter $(VARS),$(.VARIABLES)),$(.VARIABLES))), \
 		$(if $(filter-out environment% default automatic, \
 				$(origin $V)), \
 		$(if $(QUOTED_VARS),\
 			$(info $V='$(subst ','\'',$(if $(RAW_VARS),$(value $V),$($V)))'), \
-			$(info $V=$(if $(RAW_VARS),$(value $V),$($V))))))
+			$(info $V=$(if $(RAW_VARS),$(value $V),$($V)))))))
 # ' Syntax colouring...
 
 .PHONY: clean

Signature · 1 line

2.14.4

Yann E. MORIN

2018-08-29 20:49 UTC #1 lore raw

PATCH 1/1 Fix issue with printvars executing giant shell command

Trent, All,

On 2018-08-17 16:15 -0700, Trent Piepho spake thusly:

Quoted 33 lines

The underlying problem is that $(foreach V,1 2 3,) does not evaluate to an empty string. It evaluates to "? ", three empty strings separated by whitespace.
A construct of this format, with a giant list in the foreach, is part of the printvars command. This means that "@:$(foreach ....)", which is
intended to expand to a null command, in fact expands to "@: "
with a great deal of whitespace. Make chooses to execute this command with:
    execve("/bin/sh", ["/bin/sh", "-c", ":       "]
But with far more whitespace. So much that it can exceed shell command line length limits.
The solution here is to use $(strip $(foreach ...)), so the command expands to "@:", which make is smart enough to not even execute and wouldn't exceed any limits if it did.
In some cases, make will not invoke a command via "sh -c" and will exec it directly. In this case make does command line argument parsing and will swallow all the whitespace without overflowing a /bin/sh limit. It proved difficult to write a command that ensured make would do this.
Signed-off-by Trent Piepho <[email protected]>
---
 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Makefile +0 -0
index 0115c4dfcc..b396c56042 100644

Makefile $(BUILD_DIR)/.br2-external.in: $(BUILD_DIR) · line 991

Quoted 5 lines

Makefile +1 -1

@@ -988,13 +988,13 @@ $(BUILD_DIR)/.br2-external.in: $(BUILD_DIR)
 # displayed.
 .PHONY: printvars
 printvars:
-	@:$(foreach V, \
+	@:$(strip $(foreach V, \

Why do we even need an actual command here? It works exactly the same without a rule, and this indeed also gets rid of a call to a shell (and consequently won't exceed the command line length limit).

I.e. the following:

    diff --git a/Makefile b/Makefile
    index 413ec921cd..b7b1257ea6 100644
    --- a/Makefile
    +++ b/Makefile
    @@ -977,7 +977,7 @@ $(BUILD_DIR)/.br2-external.in: $(BUILD_DIR)
     # displayed.
     .PHONY: printvars
     printvars:
    -	@:$(foreach V, \
    +	$(foreach V, \
     		$(sort $(if $(VARS),$(fil		ter $(VARS),$(.VARIABLES)),$(.VARIABLES))), \
     		$(if $(filter-out environment% default automatic, \
     				$(origin $V)), \

Do you see a case where that would not work?

FTR I tried to strace it to find the shells;

    $ strace -o m.log -ff -e trace=process -- make -s printvars >/dev/null
    $ grep execve m.log* |grep '                  '
    --> nothing

(before the patch above, there was one hit on the grep.)

Regards,
Yann E. MORIN.

Quoted 17 lines

Makefile +1 -1

 		$(sort $(if $(VARS),$(filter $(VARS),$(.VARIABLES)),$(.VARIABLES))), \
 		$(if $(filter-out environment% default automatic, \
 				$(origin $V)), \
 		$(if $(QUOTED_VARS),\
 			$(info $V='$(subst ','\'',$(if $(RAW_VARS),$(value $V),$($V)))'), \
-			$(info $V=$(if $(RAW_VARS),$(value $V),$($V))))))
+			$(info $V=$(if $(RAW_VARS),$(value $V),$($V)))))))
 # ' Syntax colouring...
 
 .PHONY: clean

Signature · 6 lines

2.14.4

_______________________________________________
buildroot mailing list
buildroot at busybox.net
http://lists.busybox.net/mailman/listinfo/buildroot

Signature · 6 lines

.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

Trent Piepho

author

2018-09-10 17:45 UTC #2 lore raw

PATCH 1/1 Fix issue with printvars executing giant shell command

Quoted 6 lines · Yann E. MORIN

On 2018-08-17 16:15 -0700, Trent Piepho spake thusly:
Quoted 4 lines
The solution here is to use $(strip $(foreach ...)), so the command expands to "@:", which make is smart enough to not even execute and wouldn't exceed any limits if it did.

Quoted 12 lines

Makefile $(BUILD_DIR)/.br2-external.in: $(BUILD_DIR) · line 991
Quoted 8 lines
Makefile +1 -1
@@ -988,13 +988,13 @@ $(BUILD_DIR)/.br2-external.in: $(BUILD_DIR)
 # displayed.
 .PHONY: printvars
 printvars:
-	@:$(foreach V, \
+	@:$(strip $(foreach V, \
Why do we even need an actual command here? It works exactly the same without a rule, and this indeed also gets rid of a call to a shell (and consequently won't exceed the command line length limit).

Not quite exactly the same. There's no longer a recipe at all, only the definition of a dependency for printvars, so the output will include:

make[1]: Nothing to be done for 'printvars'.

Which isn't expected output for something that was parsing printvars.

I'll also mention that we saw this error running buildroot in certain docker containers, but not everywhere. Whether or not make will pass a giant sequence of whitespace to a shell ends up not being so simple, and I couldn't find any precise rules that govern it. So just because it works as desired in one test doesn't mean is must work that way everywhere.

However, it is clearly defined that strip will convert a sequence of whitespace to an empty string, so I figured better to relay on that.

Yann E. MORIN

2018-09-10 18:18 UTC #3 lore raw

PATCH 1/1 Fix issue with printvars executing giant shell command

Trent, All,

On 2018-09-10 17:45 +0000, Trent Piepho spake thusly:

Quoted 20 lines · Yann E. MORIN

Quoted 6 lines
On 2018-08-17 16:15 -0700, Trent Piepho spake thusly:
Quoted 4 lines
The solution here is to use $(strip $(foreach ...)), so the command expands to "@:", which make is smart enough to not even execute and wouldn't exceed any limits if it did.
Quoted 12 lines
Makefile $(BUILD_DIR)/.br2-external.in: $(BUILD_DIR) · line 991
Quoted 8 lines
Makefile +1 -1
@@ -988,13 +988,13 @@ $(BUILD_DIR)/.br2-external.in: $(BUILD_DIR)
 # displayed.
 .PHONY: printvars
 printvars:
-	@:$(foreach V, \
+	@:$(strip $(foreach V, \
Why do we even need an actual command here? It works exactly the same without a rule, and this indeed also gets rid of a call to a shell (and consequently won't exceed the command line length limit).

Quoted 5 lines

Not quite exactly the same. There's no longer a recipe at all, only the definition of a dependency for printvars, so the output will include:
make[1]: Nothing to be done for 'printvars'.

So what about:

Makefile +2 -1

index 2c6af12989..57e776056c 100644
@@ -988,7 +988,8 @@ $(BUILD_DIR)/.br2-external.in: $(BUILD_DIR)
 # displayed.
 .PHONY: printvars
 printvars:
-	@:$(foreach V, \
+	@:
+	$(foreach V, \
 		$(sort $(if $(VARS),$(filter $(VARS),$(.VARIABLES)),$(.VARIABLES))), \
 		$(if $(filter-out environment% default automatic, \
 				$(origin $V)), \

Quoted 1 line

Which isn't expected output for something that was parsing printvars.

Which should probably have already called it with 'make -s' because that is what is also needed when printvars is called from an out-of-tree build, by the way.

Quoted 6 lines

I'll also mention that we saw this error running buildroot in certain docker containers, but not everywhere. Whether or not make will pass a giant sequence of whitespace to a shell ends up not being so simple, and I couldn't find any precise rules that govern it. So just because it works as desired in one test doesn't mean is must work that way everywhere.

Before my suggested hange:

    $ strace -ff -s 1048576 -o n.log -e trace=process -- make printvars >/dev/null
    $ grep execve n.log* |grep '                  '
    --> hit!

With the above change:

    $ strace -ff -s 1048576 -o n.log -e trace=process -- make printvars >/dev/null
    $ grep execve n.log* |grep '                  '
    --> no hit!

In a recipe, a command line that contains only spaces will not be executed by make.

So with the above, you get the silence from a rule, and no long-shell commands that break.

Quoted 2 lines

However, it is clearly defined that strip will convert a sequence of whitespace to an empty string, so I figured better to relay on that.

I'm just trying to keep it simple.

Regards,
Yann E. MORIN.

Signature · 6 lines

.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

Trent Piepho

author

2018-09-17 18:02 UTC #4 lore raw

PATCH 1/1 Fix issue with printvars executing giant shell command

Quoted 28 lines · Yann E. MORIN

On 2018-09-10 17:45 +0000, Trent Piepho spake thusly:
Quoted 7 lines · Yann E. MORIN
Quoted 6 lines
On 2018-08-17 16:15 -0700, Trent Piepho spake thusly:
Quoted 5 lines
The solution here is to use $(strip $(foreach ...)), so the command expands to "@:", which make is smart enough to not even execute and wouldn't exceed any limits if it did.
So what about:
Makefile +2 -1
index 2c6af12989..57e776056c 100644
@@ -988,7 +988,8 @@ $(BUILD_DIR)/.br2-external.in: $(BUILD_DIR)
 # displayed.
 .PHONY: printvars
 printvars:
-	@:$(foreach V, \
+	@:
+	$(foreach V, \
 		$(sort $(if $(VARS),$(filter $(VARS),$(.VARIABLES)),$(.VARIABLES))), \
 		$(if $(filter-out environment% default automatic, \
 				$(origin $V)), \
In a recipe, a command line that contains only spaces will not be executed by make.

Quoted 7 lines

So with the above, you get the silence from a rule, and no long-shell commands that break.
Quoted 2 lines
However, it is clearly defined that strip will convert a sequence of whitespace to an empty string, so I figured better to relay on that.
I'm just trying to keep it simple.

Ok, if you like that better. My thought was "strip eats whitespace" is simple, but "a recipe line starting with a tab and containing only whitespace is skipped" is more obscure. In fact, while it does appear to be true, I haven't found anything in the make docs clearly stating this.

Yann E. MORIN

2018-09-17 19:54 UTC #5 lore raw

PATCH 1/1 Fix issue with printvars executing giant shell command

Trent, All,

On 2018-09-17 18:02 +0000, Trent Piepho spake thusly:

Quoted 3 lines · Yann E. MORIN

Quoted 2 lines
In a recipe, a command line that contains only spaces will not be executed by make.

[--SNIP--]

Quoted 5 lines

Ok, if you like that better. My thought was "strip eats whitespace" is simple, but "a recipe line starting with a tab and containing only whitespace is skipped" is more obscure. In fact, while it does appear to be true, I haven't found anything in the make docs clearly stating this.

From the make manual, chapter 5.1, "Recipe Syntax" [0], states:

* A blank line that begins with a tab is not blank: it?s an empty recipe (see Empty Recipes).

... where "Empty Recipes" is a link to chapter 5.9, titled "Using Empty Recipes" [1], which starts with:

It is sometimes useful to define recipes which do nothing. This is done simply by giving a recipe that consists of nothing but whitespace.

[0] https://www.gnu.org/software/make/manual/make.html#Recipe-Syntax
[1] https://www.gnu.org/software/make/manual/make.html#Empty-Recipes

Regards,
Yann E. MORIN.

Signature · 6 lines

.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'