Re: HEAD: func_show_eval shell expansion issue

[Noah Misch]

Hi Ralf,

On Fri, Aug 26, 2005 at 10:38:46AM +0200, Ralf Wildenhues wrote:
> * Noah Misch wrote on Fri, Aug 26, 2005 at 03:46:51AM CEST:
> > On Thu, Aug 25, 2005 at 04:54:06PM +0200, Ralf Wildenhues wrote:
> > > * Noah Misch wrote on Thu, Aug 25, 2005 at 03:42:34PM CEST:

> > If you simply echo the commands, you see `gcc $flags -c foo$bar.c'.  With a
> > robust func_quote_for_expand implementation, you see `gcc -Wl,bad"char -c
> > foo$bar.c', which is more descriptive.
> 
> That is not what you want to see!  Users want something
> copy-and-paste-able.  In your example, they want
>   gcc -Wl,bad\"char -c foo\$bar.c
> or 
>   gcc "-Wl,bad\"char" -c "foo\$bar.c"
> or even
>   gcc '-Wl,bad"char' -c 'foo$bar.c'

ACK.

> I am not talking about this issue!  It is closely connected, but the
> issue to which you posted links is a different one:
> 
> If I have a tag variable like (quoted as in the beginning of the libtool
> script):
>   foo_cmds="\$CC \$some_flag -o \$output ..."
> 
> and also I have
>   some_flag="\${wl}-whatever"
> 
> then, before I start any evaluation on $foo_cmds, I _need_ to eval some
> variation of $some_flag once, so that pesky ${wl} is replaced.  I cannot
> achieve this by evaluation of $foo_cmds once more than would otherwise
> be necessary, because that might just destroy other parts of the command
> line.

I agree with your analysis.  In the links I posted, there was no actual bug,
only the appearance of one.  I do not know libtool well enough to guess whether
it has bugs of this nature.

This is orthogonal to the bug yielding the output problem you reported.

> > I do not know how many func_show_eval call sites `eval' strings bearing
> > parameter expansions, and thereby benefit from this.
> > 
> > > Not in all the
> > >   cmds=$foobar_cmds
> > >   eval cmds=\"$cmds\"
> > >   ..
> > >   IFS='~'
> > >   func_show_eval "$cmds"
> > >  ..
> > > 
> > > 
> > > loops, they aren't.  And in fact, should libtool ever support objects
> > > with dollar signs in their names, they mustn't either (surely this is
> > > post next stable release).
> > 
> > Yes; that `eval' looks shaky.
> 
> Which one?  The one that happens logically first?

Yes.

> It's how things have always been in the libtool script.

Okay; I studied branch-2-0, and you are right.  The `eval' is essentially safe,
because the foo_cmds that would otherwise break are written to expect two levels
of `eval'uation.  Er, sort of; most commands don't care.  finish_cmds does care
and appears to work by near-accident.  Cygwin postinstall_cmds does care and
seems to work by design.  You seem to be saying that this dual-`eval'uation,
though longstanding, is harmful.

> > >   eval "allow_undefined_flag=\"$allow_undefined_flag\""
> > 
> > This is syntactically invalid if $allow_undefined_flag, is, say, `foo"bar'. 
> >  It
> > silently does the wrong thing on `-Wl,'strange$flag''.  
> > func_quote_for_expand
> > aims to handle cases like those.
> 
> Heck, then we _may_ need to expand-quote $allow_undefined_flag before
> this eval.  But we still need to do this evaluation on this flag alone!
> 
> And besides: we have control over the possible contents of
> $allow_undefined_flag.  We can _know_ whether it may contain any fun
> variables.  Similarly with most other tag variables that need this sort
> of treatment.

ACK.

> > > > What do you mean, different levels of expanded-ness?
> > > 
> > > Just the above.  Having $cmds eval'ed once in the main code *and* once
> > > in func_show_eval is wrong.

ACK.

> > > You can't find a regex or sed script
> > > that'll undo that and keep you cozy and warm unless you make assumptions
> > > about what the user gives you on the command line.  Not if you allow 
> > > any kinds of fun stuff in $libobjs, e.g., `$' in object file names.
> > > 
> > > Would you agree with this judgement?

Even if one could write such a thing, it seems better to remove the extra level
of evaluation, for that also makes libtool.m4 easier to write and read.

> > Yes, I bet the double eval is wrong.  No, I think someone could make a `sed'
> > script that still does the right thing.
> 
> Maybe.  But the script snippet you posted as well makes me fear this
> will have a significant time overhead.  :-/
> 
> > Not me, though.
> 
> Yep, I understood your other post.  :-/

Here, I refer to my lack of `sed' expertise.  I would surely write it in shell.

> > Note that only a handful of call sites have the double eval.
> 
> Don't be fooled, there are quite a bit of them, only I wrapped them
> through func_execute_cmds in HEAD.

I was fooled.  At least 1/3 of the call sites are affected.

---

Let me try to summarize the situation.  In branch-2-0, we use this idiom:

  cmd=\"$cmd\"
  $show "$cmd"
  eval "$cmd" || exit $?

In HEAD, we use this idiom:

  cmd=\"$cmd\"
  func_show_eval "$cmd" "${2-:}"

Both run the same commands for a given $cmd.  The level of evaluation did not
change.  What did change is the output we produce for a given $cmd.  The HEAD
output code is certainly dissatisfying, particularly because it actively
misleads when it mistreats a `$' as starting a parameter expansion.

I understand you to say that we should change everything to evaluate each
command once and selectively evaluate other variables to keep things correct.
Where we currently do something like this,

  # libtool.m4
  foo_cmd='\$tool \$flag'
  flag='${wl}-whatever'
  wl=-Wl,
  # ltmain.m4sh
  eval cmd=\"$foo_cmd\"
  $show "$cmd"
  $run eval "$cmd" || exit $?

we should instead write this:

  # libtool.m4
  foo_cmd='$tool $flag'
  flag='${wl}-whatever'
  wl=-Wl,
  # ltmain.m4sh
  eval flag=\"$flag\"
  ...
  cmd="$foo_cmd"
  $show "$cmd"
  $run eval "$cmd" || exit $?

The example is in the style of branch-2-0, but the conversion to HEAD-style is
obvious and purely cosmetic.  Also, the example is entirely fictitious, because
the good real example, the Cygwin postinstall_cmds, is too hard on my brain.

How fully does that summary intersect your understanding?