| Message ID | 20180817231548.29867-1-tpiepho@impinj.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix issue with printvars executing giant shell command | expand |
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@busybox.net > http://lists.busybox.net/mailman/listinfo/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.
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.
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.
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.
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
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(-)