zsh-workers
 help / color / mirror / code / Atom feed
* Bug in Functions/Misc/regexp-replace
@ 2021-04-29 23:53 Jacob Menke
  2021-04-30  0:40 ` Matthew Martin
  2021-04-30  6:51 ` Stephane Chazelas
  0 siblings, 2 replies; 9+ messages in thread
From: Jacob Menke @ 2021-04-29 23:53 UTC (permalink / raw)
  To: zsh-workers


[-- Attachment #1.1: Type: text/plain, Size: 478 bytes --]

Hello,

I think I found a bug in Functions/Misc/regexp-replace

Steps to reproduce:

zsh -f
str='x :=bad'
autoload regexp-replace
regexp-replace str 'a' 'z' && echo $str

Actual Output:
(eval):1: bzd not found

Expected:
x :=bzd

Root cause:
Line 41: eval ${1}=${(q)5}
It appears the ${(q)5} is not escaping = so the =command is executed after :

Instead of 'x\ :\=bad', you get '= x\ :=bad'

One way to fix:
41: eval ${1}=${(qqq)5}

Patch file is attached.

Thanks
Jacob Menke

[-- Attachment #1.2: Type: text/html, Size: 941 bytes --]

[-- Attachment #2: regexp-replace.patch --]
[-- Type: application/x-patch, Size: 328 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Bug in Functions/Misc/regexp-replace
  2021-04-29 23:53 Bug in Functions/Misc/regexp-replace Jacob Menke
@ 2021-04-30  0:40 ` Matthew Martin
  2021-04-30  5:56   ` Stephane Chazelas
  2021-04-30  6:51 ` Stephane Chazelas
  1 sibling, 1 reply; 9+ messages in thread
From: Matthew Martin @ 2021-04-30  0:40 UTC (permalink / raw)
  To: zsh-workers

On Thu, Apr 29, 2021 at 07:53:52PM -0400, Jacob Menke wrote:
> Hello,
> 
> I think I found a bug in Functions/Misc/regexp-replace

> One way to fix:
> 41: eval ${1}=${(qqq)5}
> 
> Patch file is attached.

Thanks for the report, minimal reproducer, and patch. I think it would
make sense to swap the eval for ${(P) ::= here as below.


diff --git a/Functions/Misc/regexp-replace b/Functions/Misc/regexp-replace
index dec105524..ff82c39b6 100644
--- a/Functions/Misc/regexp-replace
+++ b/Functions/Misc/regexp-replace
@@ -38,6 +38,6 @@ while [[ -n $4 ]]; do
 done
 5+=$4
 
-eval ${1}=${(q)5}
+: ${(P)1::=$5}
 # status 0 if we did something, else 1.
 [[ -n $6 ]]


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Bug in Functions/Misc/regexp-replace
  2021-04-30  0:40 ` Matthew Martin
@ 2021-04-30  5:56   ` Stephane Chazelas
  0 siblings, 0 replies; 9+ messages in thread
From: Stephane Chazelas @ 2021-04-30  5:56 UTC (permalink / raw)
  To: zsh-workers

2021-04-29 19:40:41 -0500, Matthew Martin:
> On Thu, Apr 29, 2021 at 07:53:52PM -0400, Jacob Menke wrote:
> > Hello,
> > 
> > I think I found a bug in Functions/Misc/regexp-replace
> 
> > One way to fix:
> > 41: eval ${1}=${(qqq)5}
> > 
> > Patch file is attached.
> 
> Thanks for the report, minimal reproducer, and patch. I think it would
> make sense to swap the eval for ${(P) ::= here as below.
[...]

I had a patch fixing that and some other issues in that function
in https://www.zsh.org/mla/workers/2020/msg00009.html
but it seems it wasn't applied.

-- 
Stephane


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Bug in Functions/Misc/regexp-replace
  2021-04-29 23:53 Bug in Functions/Misc/regexp-replace Jacob Menke
  2021-04-30  0:40 ` Matthew Martin
@ 2021-04-30  6:51 ` Stephane Chazelas
  2021-04-30  8:17   ` tilde expansion after quoted : in assignments Stephane Chazelas
                     ` (2 more replies)
  1 sibling, 3 replies; 9+ messages in thread
From: Stephane Chazelas @ 2021-04-30  6:51 UTC (permalink / raw)
  To: Jacob Menke; +Cc: zsh-workers

2021-04-29 19:53:52 -0400, Jacob Menke:
[...]
> regexp-replace str 'a' 'z' && echo $str
> 
> Actual Output:
> (eval):1: bzd not found
> 
> Expected:
> x :=bzd
[...]

One might argue there's a problem with the (q) parameter
expansion flag, it escapes leading =s but not the =s that follow
: even though they're special there in assignments.

$ echo a=x:=y
a=x:=y
$ a=x:=y
zsh: y not found

BTW, zsh is the only shell where ~ is expanded in:

$ zsh -c 'a=a\:~; echo $a'
a:/home/chazelas

[...]
> One way to fix:
> 41: eval ${1}=${(qqq)5}

The safest quoting operator is the (qq) one. I wouldn't use any
other for things to be reinput to the shell.

See
https://unix.stackexchange.com/questions/379181/escape-a-variable-for-use-as-content-of-another-script/600214#600214
for details on that.

In particular qqq uses double quotes inside which \ and ` are
still special and those characters also appear in the encoding
of some other characters in some locales.

But here, the best thing to do is to not expose the parser to
the contents of $5 by doing:

eval "$1=\$5"

(which tells the shell to evaluate varname=$5)

You need to expand $1 here which contains the variable name.

Note that as already noted at
https://www.zsh.org/mla/workers/2019/msg01113.html
whether you use that or

: ${(P)1::="$5"}

You'll still have a command injection vulnerability if $1 is not
guaranteed to be a variable name.

-- 
Stephane


^ permalink raw reply	[flat|nested] 9+ messages in thread

* tilde expansion after quoted : in assignments
  2021-04-30  6:51 ` Stephane Chazelas
@ 2021-04-30  8:17   ` Stephane Chazelas
  2021-04-30 17:43     ` Bart Schaefer
  2021-04-30 20:13   ` Bug in Functions/Misc/regexp-replace Jacob Menke
  2021-04-30 20:43   ` Bart Schaefer
  2 siblings, 1 reply; 9+ messages in thread
From: Stephane Chazelas @ 2021-04-30  8:17 UTC (permalink / raw)
  To: Zsh hackers list

2021-04-30 07:51:23 +0100, Stephane Chazelas:
[...]
> BTW, zsh is the only shell where ~ is expanded in:
> 
> $ zsh -c 'a=a\:~; echo $a'
> a:/home/chazelas
[...]

That's a POSIX non-conformance btw:

https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/utilities/V3_chap02.html#tag_18_06_01

Tilde expansion is only meant to be done after unquoted : per
POSIX. Though I can't imagine being a problem in practice. If
people don't want tilde expansion, they'd quote the ~, not the
:.

Also, there's a lot of variation between shells in how tilde
expansion is done and the specification is quite vague there.

See also ~$user or ~"user" or var=foo$COLON~ or var=foo${-+:}~
...

See also https://www.austingroupbugs.net/view.php?id=1172
(I had completely forgotten I'd raised that).

-- 
Stephane


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: tilde expansion after quoted : in assignments
  2021-04-30  8:17   ` tilde expansion after quoted : in assignments Stephane Chazelas
@ 2021-04-30 17:43     ` Bart Schaefer
  0 siblings, 0 replies; 9+ messages in thread
From: Bart Schaefer @ 2021-04-30 17:43 UTC (permalink / raw)
  To: Zsh hackers list

On Fri, Apr 30, 2021 at 1:18 AM Stephane Chazelas <stephane@chazelas.org> wrote:
>
> Tilde expansion is only meant to be done after unquoted : per
> POSIX. Though I can't imagine being a problem in practice.

I was expecting this to mean that colons in e.g. $PATH could be
escaped with a backslash, but that's not the case.

Therefore I suppose it's so that one can do
  a=${this}\:${that}
without having to worry whether $that begins with a tilde, while
simultaneously allowing ${that} to be whitespace-split or to itself
contain unquoted colons.  Although again that seems a pretty weird
case since the only reason to care whether those colons are quoted is
if they are followed by more tildes and I can't imagine what splitting
makes sense in that context.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Bug in Functions/Misc/regexp-replace
  2021-04-30  6:51 ` Stephane Chazelas
  2021-04-30  8:17   ` tilde expansion after quoted : in assignments Stephane Chazelas
@ 2021-04-30 20:13   ` Jacob Menke
  2021-04-30 21:22     ` Bart Schaefer
  2021-04-30 20:43   ` Bart Schaefer
  2 siblings, 1 reply; 9+ messages in thread
From: Jacob Menke @ 2021-04-30 20:13 UTC (permalink / raw)
  To: Jacob Menke, stephane; +Cc: zsh-workers

[-- Attachment #1: Type: text/plain, Size: 1673 bytes --]

Awesome, thanks for the expert response!

I vote for

eval "$1=\$5"

Jacob

On Fri, 30 Apr 2021 at 02:51, Stephane Chazelas <stephane@chazelas.org>
wrote:

> 2021-04-29 19:53:52 -0400, Jacob Menke:
> [...]
> > regexp-replace str 'a' 'z' && echo $str
> >
> > Actual Output:
> > (eval):1: bzd not found
> >
> > Expected:
> > x :=bzd
> [...]
>
> One might argue there's a problem with the (q) parameter
> expansion flag, it escapes leading =s but not the =s that follow
> : even though they're special there in assignments.
>
> $ echo a=x:=y
> a=x:=y
> $ a=x:=y
> zsh: y not found
>
> BTW, zsh is the only shell where ~ is expanded in:
>
> $ zsh -c 'a=a\:~; echo $a'
> a:/home/chazelas
>
> [...]
> > One way to fix:
> > 41: eval ${1}=${(qqq)5}
>
> The safest quoting operator is the (qq) one. I wouldn't use any
> other for things to be reinput to the shell.
>
> See
>
> https://unix.stackexchange.com/questions/379181/escape-a-variable-for-use-as-content-of-another-script/600214#600214
> for details on that.
>
> In particular qqq uses double quotes inside which \ and ` are
> still special and those characters also appear in the encoding
> of some other characters in some locales.
>
> But here, the best thing to do is to not expose the parser to
> the contents of $5 by doing:
>
> eval "$1=\$5"
>
> (which tells the shell to evaluate varname=$5)
>
> You need to expand $1 here which contains the variable name.
>
> Note that as already noted at
> https://www.zsh.org/mla/workers/2019/msg01113.html
> whether you use that or
>
> : ${(P)1::="$5"}
>
> You'll still have a command injection vulnerability if $1 is not
> guaranteed to be a variable name.
>
> --
> Stephane
>

[-- Attachment #2: Type: text/html, Size: 2596 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Bug in Functions/Misc/regexp-replace
  2021-04-30  6:51 ` Stephane Chazelas
  2021-04-30  8:17   ` tilde expansion after quoted : in assignments Stephane Chazelas
  2021-04-30 20:13   ` Bug in Functions/Misc/regexp-replace Jacob Menke
@ 2021-04-30 20:43   ` Bart Schaefer
  2 siblings, 0 replies; 9+ messages in thread
From: Bart Schaefer @ 2021-04-30 20:43 UTC (permalink / raw)
  To: Jacob Menke, Zsh hackers list

On Thu, Apr 29, 2021 at 11:51 PM Stephane Chazelas
<stephane@chazelas.org> wrote:
>
> One might argue there's a problem with the (q) parameter
> expansion flag, it escapes leading =s but not the =s that follow
> : even though they're special there in assignments.

Arguably (q) should either quote all =s or none of them.  It has no
idea what appears outside the parameter reference, that is, there's no
reason for it to treat a leading = as being the start of a word in the
surrounding context (or to believe the context is assignment).  I
can't think of any situation where I've depended upon the
quotes-leading-equal behavior.

On the other hand the check for a leading equal means that (q-) would
work here (and is frequently better than just (q) for other reasons).


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Bug in Functions/Misc/regexp-replace
  2021-04-30 20:13   ` Bug in Functions/Misc/regexp-replace Jacob Menke
@ 2021-04-30 21:22     ` Bart Schaefer
  0 siblings, 0 replies; 9+ messages in thread
From: Bart Schaefer @ 2021-04-30 21:22 UTC (permalink / raw)
  To: Jacob Menke; +Cc: stephane, Zsh hackers list

On Fri, Apr 30, 2021 at 1:14 PM Jacob Menke <linux.dev25@gmail.com> wrote:
>
> I vote for
>
> eval "$1=\$5"

I'd actually go for

typeset -g $1=$5

and avoid the eval entirely.


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-04-30 21:23 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-29 23:53 Bug in Functions/Misc/regexp-replace Jacob Menke
2021-04-30  0:40 ` Matthew Martin
2021-04-30  5:56   ` Stephane Chazelas
2021-04-30  6:51 ` Stephane Chazelas
2021-04-30  8:17   ` tilde expansion after quoted : in assignments Stephane Chazelas
2021-04-30 17:43     ` Bart Schaefer
2021-04-30 20:13   ` Bug in Functions/Misc/regexp-replace Jacob Menke
2021-04-30 21:22     ` Bart Schaefer
2021-04-30 20:43   ` Bart Schaefer

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/zsh/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).