[Bug] incorrect warning when I type rm /*

[Bug] incorrect warning when I type rm /*

[Jonas Bräutigam]

Hello,

these days I wrote a command wrong. Instead of

"$ rm ./*" <- deleting  in the current directory

I wrote:

"$ rm /*" <- deleting in the root path "/"

zsh warn me with:

"zsh: sure you want to delete all 4 files in /home/[user]/Downloads [yn]?"

After I typed "y", it tried to delete the files in "/".

So the correct warning should be:

"zsh: sure you want to delete all X files in / [yn]?"


I use debian 10.4 with no "rm"-specifiy plugin.


Whatch out If you try it! :D


Best Regards

Jonas

Re: [Bug] incorrect warning when I type rm /*

[Daniel Shahaf]

Jonas Bräutigam wrote on Wed, 01 Jul 2020 12:02 +0200:
> Hello,
> 
> these days I wrote a command wrong. Instead of
> 
> "$ rm ./*" <- deleting  in the current directory
> 
> I wrote:
> 
> "$ rm /*" <- deleting in the root path "/"
> 
> zsh warn me with:
> 
> "zsh: sure you want to delete all 4 files in /home/[user]/Downloads [yn]?"
> 
> After I typed "y", it tried to delete the files in "/".
> 
> So the correct warning should be:
> 
> "zsh: sure you want to delete all X files in / [yn]?"
> 
> 
> I use debian 10.4 with no "rm"-specifiy plugin.
> 
> 
> Whatch out If you try it! :D

Thanks; it's a path arithmetic edge case:

diff --git a/Src/exec.c b/Src/exec.c
index 045b5d2b9..7120a2c34 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -3401,7 +3401,7 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 		int rmall;
 
 		s[l - 2] = 0;
-		rmall = checkrmall(s);
+		rmall = checkrmall(l == 2 ? "/" : s);
 		s[l - 2] = t;
 
 		if (!rmall) {

The "Is the dir empty?" logic likewise operated on $PWD rather than /,
so if someone runs «rm /*» as root in an empty directory, the
RM_STAR_SILENT confirmation prompt would not appear at all.

I haven't figured out how to write a test for this (short of using
expect(1) or so).

Cheers,

Daniel

Re: [Bug] incorrect warning when I type rm /*

[Mikael Magnusson]

On 7/1/20, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> Jonas Bräutigam wrote on Wed, 01 Jul 2020 12:02 +0200:
>> Hello,
>>
>> these days I wrote a command wrong. Instead of
>>
>> "$ rm ./*" <- deleting  in the current directory
>>
>> I wrote:
>>
>> "$ rm /*" <- deleting in the root path "/"
>>
>> zsh warn me with:
>>
>> "zsh: sure you want to delete all 4 files in /home/[user]/Downloads
>> [yn]?"
>>
>> After I typed "y", it tried to delete the files in "/".
>>
>> So the correct warning should be:
>>
>> "zsh: sure you want to delete all X files in / [yn]?"
>>
>>
>> I use debian 10.4 with no "rm"-specifiy plugin.
>>
>>
>> Whatch out If you try it! :D
>
> Thanks; it's a path arithmetic edge case:
>
> diff --git a/Src/exec.c b/Src/exec.c
> index 045b5d2b9..7120a2c34 100644
> --- a/Src/exec.c
> +++ b/Src/exec.c
> @@ -3401,7 +3401,7 @@ execcmd_exec(Estate state, Execcmd_params eparams,
>  		int rmall;
>
>  		s[l - 2] = 0;
> -		rmall = checkrmall(s);
> +		rmall = checkrmall(l == 2 ? "/" : s);
>  		s[l - 2] = t;
>
>  		if (!rmall) {
>
> The "Is the dir empty?" logic likewise operated on $PWD rather than /,
> so if someone runs «rm /*» as root in an empty directory, the
> RM_STAR_SILENT confirmation prompt would not appear at all.
>
> I haven't figured out how to write a test for this (short of using
> expect(1) or so).

zpty_start and variants as in W02jobs, X02zlevi and others?

-- 
Mikael Magnusson

Re: [Bug] incorrect warning when I type rm /*

[Daniel Shahaf]

Mikael Magnusson wrote on Wed, 01 Jul 2020 13:33 +0200:
> On 7/1/20, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > Jonas Bräutigam wrote on Wed, 01 Jul 2020 12:02 +0200:  
> >> Hello,
> >>
> >> these days I wrote a command wrong. Instead of
> >>
> >> "$ rm ./*" <- deleting  in the current directory
> >>
> >> I wrote:
> >>
> >> "$ rm /*" <- deleting in the root path "/"
> >>
> >> zsh warn me with:
> >>
> >> "zsh: sure you want to delete all 4 files in /home/[user]/Downloads
> >> [yn]?"
> >>
> >> After I typed "y", it tried to delete the files in "/".
> >>
> >> So the correct warning should be:
> >>
> >> "zsh: sure you want to delete all X files in / [yn]?"
> >>
> >>
> >> I use debian 10.4 with no "rm"-specifiy plugin.
> >>
> >>
> >> Whatch out If you try it! :D  
> >
> > Thanks; it's a path arithmetic edge case:
> >
> > diff --git a/Src/exec.c b/Src/exec.c
> > index 045b5d2b9..7120a2c34 100644
> > --- a/Src/exec.c
> > +++ b/Src/exec.c
> > @@ -3401,7 +3401,7 @@ execcmd_exec(Estate state, Execcmd_params eparams,
> >  		int rmall;
> >
> >  		s[l - 2] = 0;
> > -		rmall = checkrmall(s);
> > +		rmall = checkrmall(l == 2 ? "/" : s);
> >  		s[l - 2] = t;
> >
> >  		if (!rmall) {
> >
> > The "Is the dir empty?" logic likewise operated on $PWD rather than /,
> > so if someone runs «rm /*» as root in an empty directory, the
> > RM_STAR_SILENT confirmation prompt would not appear at all.
> >
> > I haven't figured out how to write a test for this (short of using
> > expect(1) or so).  
> 
> zpty_start and variants as in W02jobs, X02zlevi and others?

Thanks for the pointer.  I'll look when I have time, but if anyone can
beat me to it, please do.

Cheers,

Daniel

Re: [Bug] incorrect warning when I type rm /*

[Daniel Shahaf]

Mikael Magnusson wrote on Wed, 01 Jul 2020 13:33 +0200:
> On 7/1/20, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > I haven't figured out how to write a test for this (short of using
> > expect(1) or so).  
> 
> zpty_start and variants as in W02jobs, X02zlevi and others?

The following works, but I'm sure there's a more elegant way to do it.

8<--8<--
From d2aed981ed378c5634b5fcdfd578acee40d59841 Mon Sep 17 00:00:00 2001
From: Daniel Shahaf <d.s@daniel.shahaf.name>
Date: Fri, 3 Jul 2020 13:31:10 +0000
Subject: [PATCH 1/3] Add a unit test for the RM_STAR_SILENT option.

---
 Test/E01options.ztst | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/Test/E01options.ztst b/Test/E01options.ztst
index 70736f444..a1b564327 100644
--- a/Test/E01options.ztst
+++ b/Test/E01options.ztst
@@ -57,7 +57,6 @@
 #    PROMPT_CR
 #    PUSHD_SILENT
 #    REC_EXACT
-#    RM_STAR_SILENT
 #    RM_STAR_WAIT
 #    SHARE_HISTORY
 #    SINGLE_LINE_ZLE
@@ -92,6 +91,9 @@
   catpath=$(which cat)
   lspath==ls
 
+  # If the module fails to load, individual test points will skip.
+  zmodload zsh/zpty 2>/dev/null || true 
+
 %test
 
   # setopt should move on to the next operation in the face of an error, but
@@ -1423,3 +1425,26 @@ F:If this test fails at the first unsetopt, refer to P01privileged.ztst.
     (( UID == EUID ))
   fi
 0:PRIVILEGED sanity check: default value is correct
+
+  if zmodload -e zsh/zpty 2>/dev/null; then
+    for target_dir target_pattern in \
+      '.' '*'
+    do
+      before=`ls -a -- $target_dir`
+      zpty subshell $ZTST_testdir/../Src/zsh -f
+      [[ $PWD == */options.tmp ]] || return 1 # Sanity check before calling rm(1).
+      zpty -w subshell "rm $target_pattern"
+      zpty -w subshell 'n'
+      sleep 1
+      zpty -rt subshell REPLY # "${PS1} rm *"
+      zpty -rt subshell REPLY && print -r -- ${REPLY%%$'\r\n'}
+      zpty -d subshell
+      after=`ls -a -- $target_dir`
+      [[ $before == $after ]] || return 1
+    done
+  else
+    ZTST_skip="the zsh/zpty module is not available"
+  fi
+  BEL=$'\a'
+0q:RM_STAR_SILENT
+*>zsh: sure you want to delete all 15 files in ${PWD:h}/options.tmp \[yn\]\? ${BEL}

From 6a83700ee1f5f08c17c8438c66bb3fff48060182 Mon Sep 17 00:00:00 2001
From: Daniel Shahaf <d.s@daniel.shahaf.name>
Date: Fri, 3 Jul 2020 13:37:46 +0000
Subject: [PATCH 2/3] Add a regression test for 46169: the RM_STAR_SILENT logic
 processes the current directory rather than the root directory.

The bug will be fixed in the next commit.
---
 Test/E01options.ztst | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Test/E01options.ztst b/Test/E01options.ztst
index a1b564327..053affeed 100644
--- a/Test/E01options.ztst
+++ b/Test/E01options.ztst
@@ -1428,7 +1428,8 @@ F:If this test fails at the first unsetopt, refer to P01privileged.ztst.
 
   if zmodload -e zsh/zpty 2>/dev/null; then
     for target_dir target_pattern in \
-      '.' '*'
+      '.' '*' \
+      '/' '/*'
     do
       before=`ls -a -- $target_dir`
       zpty subshell $ZTST_testdir/../Src/zsh -f
@@ -1446,5 +1447,6 @@ F:If this test fails at the first unsetopt, refer to P01privileged.ztst.
     ZTST_skip="the zsh/zpty module is not available"
   fi
   BEL=$'\a'
-0q:RM_STAR_SILENT
+0qf:RM_STAR_SILENT
 *>zsh: sure you want to delete all 15 files in ${PWD:h}/options.tmp \[yn\]\? ${BEL}
+*>zsh: sure you want to delete all <-> files in / \[yn\]\? ${BEL}

From 81b1f0018d2557aa607080570ea1d78237c006c7 Mon Sep 17 00:00:00 2001
From: Daniel Shahaf <d.s@daniel.shahaf.name>
Date: Fri, 3 Jul 2020 13:38:58 +0000
Subject: [PATCH 3/3] Fix the RM_STAR_SILENT bug from the parent commit.

---
 Src/exec.c           | 2 +-
 Test/E01options.ztst | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Src/exec.c b/Src/exec.c
index 045b5d2b9..7120a2c34 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -3401,7 +3401,7 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 		int rmall;
 
 		s[l - 2] = 0;
-		rmall = checkrmall(s);
+		rmall = checkrmall(l == 2 ? "/" : s);
 		s[l - 2] = t;
 
 		if (!rmall) {
diff --git a/Test/E01options.ztst b/Test/E01options.ztst
index 053affeed..c59509f2e 100644
--- a/Test/E01options.ztst
+++ b/Test/E01options.ztst
@@ -1447,6 +1447,6 @@ F:If this test fails at the first unsetopt, refer to P01privileged.ztst.
     ZTST_skip="the zsh/zpty module is not available"
   fi
   BEL=$'\a'
-0qf:RM_STAR_SILENT
+0q:RM_STAR_SILENT
 *>zsh: sure you want to delete all 15 files in ${PWD:h}/options.tmp \[yn\]\? ${BEL}
 *>zsh: sure you want to delete all <-> files in / \[yn\]\? ${BEL}