* [Buildroot] [PATCH] Fix issue with printvars executing giant shell command
@ 2018-08-17 23:15 Trent Piepho
2018-08-29 20:49 ` Yann E. MORIN
0 siblings, 1 reply; 6+ messages in thread
From: Trent Piepho @ 2018-08-17 23:15 UTC (permalink / raw)
To: buildroot
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 <tpiepho@impinj.com>
---
Makefile | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Makefile b/Makefile
index 0115c4dfcc..b396c56042 100644
--- a/Makefile
+++ b/Makefile
@@ -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
--
2.14.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* [Buildroot] [PATCH] Fix issue with printvars executing giant shell command 2018-08-17 23:15 [Buildroot] [PATCH] Fix issue with printvars executing giant shell command Trent Piepho @ 2018-08-29 20:49 ` Yann E. MORIN 2018-09-10 17:45 ` Trent Piepho 0 siblings, 1 reply; 6+ messages in thread From: Yann E. MORIN @ 2018-08-29 20:49 UTC (permalink / raw) To: buildroot Trent, All, On 2018-08-17 16:15 -0700, Trent Piepho spake thusly: > 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 <tpiepho@impinj.com> > --- > Makefile | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/Makefile b/Makefile > index 0115c4dfcc..b396c56042 100644 > --- a/Makefile > +++ b/Makefile > @@ -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. > $(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 > -- > 2.14.4 > > _______________________________________________ > buildroot mailing list > buildroot at busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot -- .-----------------.--------------------.------------------.--------------------. | 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. | '------------------------------^-------^------------------^--------------------' ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Buildroot] [PATCH] Fix issue with printvars executing giant shell command 2018-08-29 20:49 ` Yann E. MORIN @ 2018-09-10 17:45 ` Trent Piepho 2018-09-10 18:18 ` Yann E. MORIN 0 siblings, 1 reply; 6+ messages in thread From: Trent Piepho @ 2018-09-10 17:45 UTC (permalink / raw) To: buildroot On Wed, 2018-08-29 at 22:49 +0200, Yann E. MORIN wrote: > On 2018-08-17 16:15 -0700, Trent Piepho spake thusly: > > > > 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. > > --- a/Makefile > > +++ b/Makefile > > @@ -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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Buildroot] [PATCH] Fix issue with printvars executing giant shell command 2018-09-10 17:45 ` Trent Piepho @ 2018-09-10 18:18 ` Yann E. MORIN 2018-09-17 18:02 ` Trent Piepho 0 siblings, 1 reply; 6+ messages in thread From: Yann E. MORIN @ 2018-09-10 18:18 UTC (permalink / raw) To: buildroot Trent, All, On 2018-09-10 17:45 +0000, Trent Piepho spake thusly: > On Wed, 2018-08-29 at 22:49 +0200, Yann E. MORIN wrote: > > On 2018-08-17 16:15 -0700, Trent Piepho spake thusly: > > > > > > 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. > > > > --- a/Makefile > > > +++ b/Makefile > > > @@ -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'. So what about: diff --git a/Makefile b/Makefile index 2c6af12989..57e776056c 100644 --- a/Makefile +++ b/Makefile @@ -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)), \ > 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. > 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. > 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. -- .-----------------.--------------------.------------------.--------------------. | 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. | '------------------------------^-------^------------------^--------------------' ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Buildroot] [PATCH] Fix issue with printvars executing giant shell command 2018-09-10 18:18 ` Yann E. MORIN @ 2018-09-17 18:02 ` Trent Piepho 2018-09-17 19:54 ` Yann E. MORIN 0 siblings, 1 reply; 6+ messages in thread From: Trent Piepho @ 2018-09-17 18:02 UTC (permalink / raw) To: buildroot On Mon, 2018-09-10 at 20:18 +0200, Yann E. MORIN wrote: > On 2018-09-10 17:45 +0000, Trent Piepho spake thusly: > > On Wed, 2018-08-29 at 22:49 +0200, Yann E. MORIN wrote: > > > On 2018-08-17 16:15 -0700, Trent Piepho spake thusly: > > > > > > > > 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: > > diff --git a/Makefile b/Makefile > index 2c6af12989..57e776056c 100644 > --- a/Makefile > +++ b/Makefile > @@ -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. > > So with the above, you get the silence from a rule, and no long-shell > commands that break. > > > 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Buildroot] [PATCH] Fix issue with printvars executing giant shell command 2018-09-17 18:02 ` Trent Piepho @ 2018-09-17 19:54 ` Yann E. MORIN 0 siblings, 0 replies; 6+ messages in thread From: Yann E. MORIN @ 2018-09-17 19:54 UTC (permalink / raw) To: buildroot Trent, All, On 2018-09-17 18:02 +0000, Trent Piepho spake thusly: > On Mon, 2018-09-10 at 20:18 +0200, Yann E. MORIN wrote: > > In a recipe, a command line that contains only spaces will not be executed > > by make. [--SNIP--] > 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. -- .-----------------.--------------------.------------------.--------------------. | 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. | '------------------------------^-------^------------------^--------------------' ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-09-17 19:54 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-08-17 23:15 [Buildroot] [PATCH] Fix issue with printvars executing giant shell command Trent Piepho 2018-08-29 20:49 ` Yann E. MORIN 2018-09-10 17:45 ` Trent Piepho 2018-09-10 18:18 ` Yann E. MORIN 2018-09-17 18:02 ` Trent Piepho 2018-09-17 19:54 ` Yann E. MORIN
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox