zsh-workers
 help / color / mirror / code / Atom feed
* [patch] Avoid race in zf_mkdir
@ 2020-10-09 20:07 Matthew Martin
  2020-10-09 20:24 ` Bart Schaefer
  0 siblings, 1 reply; 14+ messages in thread
From: Matthew Martin @ 2020-10-09 20:07 UTC (permalink / raw)
  To: zsh-workers

A user reported in #zsh they were seeing sporadic zf_mkdir errors when
concurrently creating the same directory. Move the stat ISDIR check to
after the mkdir call to avoid the race.


diff --git a/Src/Modules/files.c b/Src/Modules/files.c
index 6d20e38a8..ae301c14f 100644
--- a/Src/Modules/files.c
+++ b/Src/Modules/files.c
@@ -124,17 +124,17 @@ domkdir(char *nam, char *path, mode_t mode, int p)
     mode_t oumask;
     char const *rpath = unmeta(path);
 
+    oumask = umask(0);
+    err = mkdir(rpath, mode) ? errno : 0;
+    umask(oumask);
+    if(!err)
+	return 0;
     if(p) {
 	struct stat st;
 
 	if(!stat(rpath, &st) && S_ISDIR(st.st_mode))
 	    return 0;
     }
-    oumask = umask(0);
-    err = mkdir(rpath, mode) ? errno : 0;
-    umask(oumask);
-    if(!err)
-	return 0;
     zwarnnam(nam, "cannot make directory `%s': %e", path, err);
     return 1;
 }


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

* Re: [patch] Avoid race in zf_mkdir
  2020-10-09 20:07 [patch] Avoid race in zf_mkdir Matthew Martin
@ 2020-10-09 20:24 ` Bart Schaefer
  2020-10-09 20:35   ` Roman Perepelitsa
  0 siblings, 1 reply; 14+ messages in thread
From: Bart Schaefer @ 2020-10-09 20:24 UTC (permalink / raw)
  To: zsh-workers

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

On Fri, Oct 9, 2020 at 1:08 PM Matthew Martin <phy1729@gmail.com> wrote:

> A user reported in #zsh they were seeing sporadic zf_mkdir errors when
> concurrently creating the same directory. Move the stat ISDIR check to
> after the mkdir call to avoid the race.
>

Er, sorry, this doesn't actually avoid the race, it just prevents the error
message from being shown by whichever shell loses the race.

I'm not sure that's really desirable?  Sometimes you might want to know
that the directory was NOT created BY the current shell?

On the flip side in the original code we skip the mkdir entirely if the
directory already exists, so perhaps this is close enough.

In neither variation are we checking that the existing directory actually
has the requested mode.

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

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

* Re: [patch] Avoid race in zf_mkdir
  2020-10-09 20:24 ` Bart Schaefer
@ 2020-10-09 20:35   ` Roman Perepelitsa
  2020-10-09 20:47     ` Bart Schaefer
  0 siblings, 1 reply; 14+ messages in thread
From: Roman Perepelitsa @ 2020-10-09 20:35 UTC (permalink / raw)
  To: Bart Schaefer; +Cc: zsh-workers

On Fri, Oct 9, 2020 at 10:25 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> Er, sorry, this doesn't actually avoid the race, it just prevents the error message from being shown by whichever shell loses the race.

I think this is the expected behavior. It's prescribed by POSIX for mkdir.

> ISometimes you might want to know that the directory was NOT created BY the current shell?

In this case you would invoke zf_mkdir without -p.

> In neither variation are we checking that the existing directory actually has the requested mode.

POSIX says this is how it should be.

Roman.

P.S.

The patch is incorrect for a different reason. If `zf_mkdir -p foo` is
racing with another process that's doing `mkdir foo && rmdir foo`, the
zf_mkdir call must never fail but with this patch it can fail.


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

* Re: [patch] Avoid race in zf_mkdir
  2020-10-09 20:35   ` Roman Perepelitsa
@ 2020-10-09 20:47     ` Bart Schaefer
  2020-10-09 20:53       ` Matthew Martin
  0 siblings, 1 reply; 14+ messages in thread
From: Bart Schaefer @ 2020-10-09 20:47 UTC (permalink / raw)
  To: Roman Perepelitsa; +Cc: zsh-workers

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

On Fri, Oct 9, 2020 at 1:35 PM Roman Perepelitsa <
roman.perepelitsa@gmail.com> wrote:

> On Fri, Oct 9, 2020 at 10:25 PM Bart Schaefer <schaefer@brasslantern.com>
> wrote:
> >
> > Er, sorry, this doesn't actually avoid the race, it just prevents the
> error message from being shown by whichever shell loses the race.
>
> I think this is the expected behavior. It's prescribed by POSIX for mkdir.
>
[...]

>
> The patch is incorrect for a different reason. If `zf_mkdir -p foo` is
> racing with another process that's doing `mkdir foo && rmdir foo`, the
> zf_mkdir call must never fail but with this patch it can fail.
>

Hm ... in that case the code shouldn't call stat() unless the mkdir() gives
EEXIST?  And then ignore ENOENT from stat()?

What if another process is doing "touch foo && rm foo"?  How is it possible
to distinguish that from mkdir+rmdir ?

Or are we confusing the requirements for mkdir(2) from those for mkdir(1)
?  I don't have the spec handy.

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

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

* Re: [patch] Avoid race in zf_mkdir
  2020-10-09 20:47     ` Bart Schaefer
@ 2020-10-09 20:53       ` Matthew Martin
  2020-10-09 21:22         ` Roman Perepelitsa
  0 siblings, 1 reply; 14+ messages in thread
From: Matthew Martin @ 2020-10-09 20:53 UTC (permalink / raw)
  To: zsh-workers

On Fri, Oct 09, 2020 at 01:47:49PM -0700, Bart Schaefer wrote:
> On Fri, Oct 9, 2020 at 1:35 PM Roman Perepelitsa <
> roman.perepelitsa@gmail.com> wrote:
> 
> > On Fri, Oct 9, 2020 at 10:25 PM Bart Schaefer <schaefer@brasslantern.com>
> > wrote:
> > >
> > > Er, sorry, this doesn't actually avoid the race, it just prevents the
> > error message from being shown by whichever shell loses the race.
> >
> > I think this is the expected behavior. It's prescribed by POSIX for mkdir.
> >
> [...]
> 
> >
> > The patch is incorrect for a different reason. If `zf_mkdir -p foo` is
> > racing with another process that's doing `mkdir foo && rmdir foo`, the
> > zf_mkdir call must never fail but with this patch it can fail.
> >
> 
> Hm ... in that case the code shouldn't call stat() unless the mkdir() gives
> EEXIST?  And then ignore ENOENT from stat()?
> 
> What if another process is doing "touch foo && rm foo"?  How is it possible
> to distinguish that from mkdir+rmdir ?
> 
> Or are we confusing the requirements for mkdir(2) from those for mkdir(1)
> ?  I don't have the spec handy.

https://pubs.opengroup.org/onlinepubs/9699919799/utilities/mkdir.html

I suppose we could stat before and after if after mkdir errno = EEXIST.


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

* Re: [patch] Avoid race in zf_mkdir
  2020-10-09 20:53       ` Matthew Martin
@ 2020-10-09 21:22         ` Roman Perepelitsa
  2020-10-09 21:27           ` Bart Schaefer
  2020-10-09 21:40           ` Matthew Martin
  0 siblings, 2 replies; 14+ messages in thread
From: Roman Perepelitsa @ 2020-10-09 21:22 UTC (permalink / raw)
  To: Zsh hackers list

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

Perhaps something like this? This should provide the following
guarantees for zf_mkdir -p:

- If it succeeds, the directory must have existed at some point during
the execution of the function (either created by zf_mkdir itself or by
some other concurrent process).
- If it fails, there must have been a point in time during the
execution of the function where the target directory or one of its
parents didn't exist and it was impossible to create it.

`zf_mkdir -p foo` It should work as expected in the face of concurrent
`mkdir foo && rmdir foo` or `touch foo && rm foo`.

I confess that I haven't tested it.

Roman.

[-- Attachment #2: mkdir-patch.txt --]
[-- Type: text/plain, Size: 911 bytes --]

diff --git a/Src/Modules/files.c b/Src/Modules/files.c
index 6d20e38a8..a9ccccb8b 100644
--- a/Src/Modules/files.c
+++ b/Src/Modules/files.c
@@ -122,19 +122,28 @@ domkdir(char *nam, char *path, mode_t mode, int p)
 {
     int err;
     mode_t oumask;
+    struct stat st;
     char const *rpath = unmeta(path);
 
-    if(p) {
-	struct stat st;
-
-	if(!stat(rpath, &st) && S_ISDIR(st.st_mode))
+    while(1) {
+	oumask = umask(0);
+	err = mkdir(rpath, mode) ? errno : 0;
+	umask(oumask);
+	if (!err)
+	    return 0;
+	if(!p || err != EEXIST)
+	    break;
+	if(!stat(rpath, &st)) {
+	    if(errno == ENOENT)
+		continue;
+	    err = errno;
+	    break;
+	}
+	if(S_ISDIR(st.st_mode))
 	    return 0;
+	break;
     }
-    oumask = umask(0);
-    err = mkdir(rpath, mode) ? errno : 0;
-    umask(oumask);
-    if(!err)
-	return 0;
+
     zwarnnam(nam, "cannot make directory `%s': %e", path, err);
     return 1;
 }

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

* Re: [patch] Avoid race in zf_mkdir
  2020-10-09 21:22         ` Roman Perepelitsa
@ 2020-10-09 21:27           ` Bart Schaefer
  2020-10-10 11:50             ` Roman Perepelitsa
  2020-10-09 21:40           ` Matthew Martin
  1 sibling, 1 reply; 14+ messages in thread
From: Bart Schaefer @ 2020-10-09 21:27 UTC (permalink / raw)
  To: Roman Perepelitsa; +Cc: Zsh hackers list

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

On Fri, Oct 9, 2020 at 2:22 PM Roman Perepelitsa <
roman.perepelitsa@gmail.com> wrote:

> Perhaps something like this?
>

That would potentially cover it, except for one thing.  Back to an earlier
message:

> > In neither variation are we checking that the existing directory
actually has the requested mode.
>
> POSIX says this is how it should be.

The document linked by Matthew asserts that "mkdir -m mode" should behave
"as if" chmod() is called after creating the directory.

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

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

* Re: [patch] Avoid race in zf_mkdir
  2020-10-09 21:22         ` Roman Perepelitsa
  2020-10-09 21:27           ` Bart Schaefer
@ 2020-10-09 21:40           ` Matthew Martin
  1 sibling, 0 replies; 14+ messages in thread
From: Matthew Martin @ 2020-10-09 21:40 UTC (permalink / raw)
  To: zsh-workers

On Fri, Oct 09, 2020 at 11:22:00PM +0200, Roman Perepelitsa wrote:
> Perhaps something like this? This should provide the following
> guarantees for zf_mkdir -p:
> 
> - If it succeeds, the directory must have existed at some point during
> the execution of the function (either created by zf_mkdir itself or by
> some other concurrent process).
> - If it fails, there must have been a point in time during the
> execution of the function where the target directory or one of its
> parents didn't exist and it was impossible to create it.
> 
> `zf_mkdir -p foo` It should work as expected in the face of concurrent
> `mkdir foo && rmdir foo` or `touch foo && rm foo`.
> 
> I confess that I haven't tested it.
> 
> Roman.

> diff --git a/Src/Modules/files.c b/Src/Modules/files.c
> index 6d20e38a8..a9ccccb8b 100644
> --- a/Src/Modules/files.c
> +++ b/Src/Modules/files.c
> @@ -122,19 +122,28 @@ domkdir(char *nam, char *path, mode_t mode, int p)
>  {
>      int err;
>      mode_t oumask;
> +    struct stat st;
>      char const *rpath = unmeta(path);
>  
> -    if(p) {
> -	struct stat st;
> -
> -	if(!stat(rpath, &st) && S_ISDIR(st.st_mode))
> +    while(1) {
> +	oumask = umask(0);
> +	err = mkdir(rpath, mode) ? errno : 0;
> +	umask(oumask);
> +	if (!err)
> +	    return 0;
> +	if(!p || err != EEXIST)
> +	    break;
> +	if(!stat(rpath, &st)) {
> +	    if(errno == ENOENT)
> +		continue;

For a sufficiently well timed attacker, the target could be created and
deleted so that this loop never exits.  Even if pathological, I don't
think it should be possible for mkdir to loop forever.

> +	    err = errno;
> +	    break;
> +	}
> +	if(S_ISDIR(st.st_mode))
>  	    return 0;
> +	break;
>      }
> -    oumask = umask(0);
> -    err = mkdir(rpath, mode) ? errno : 0;
> -    umask(oumask);
> -    if(!err)
> -	return 0;
> +
>      zwarnnam(nam, "cannot make directory `%s': %e", path, err);
>      return 1;
>  }



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

* Re: [patch] Avoid race in zf_mkdir
  2020-10-09 21:27           ` Bart Schaefer
@ 2020-10-10 11:50             ` Roman Perepelitsa
  2020-10-15 10:01               ` Roman Perepelitsa
  0 siblings, 1 reply; 14+ messages in thread
From: Roman Perepelitsa @ 2020-10-10 11:50 UTC (permalink / raw)
  To: Bart Schaefer; +Cc: Zsh hackers list

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

On Fri, Oct 9, 2020 at 11:27 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> The document linked by Matthew asserts that "mkdir -m mode" should behave "as if" chmod() is called after creating the directory.

This applies only when mkdir creates a directory but not when the
directory already existed prior to the call. Here's the relevant part:

   Each dir operand that names an existing directory shall be ignored
without error.

On Fri, Oct 9, 2020 at 11:40 PM Matthew Martin <phy1729@gmail.com> wrote:
>
> For a sufficiently well timed attacker, the target could be created and
> deleted so that this loop never exits.  Even if pathological, I don't
> think it should be possible for mkdir to loop forever.

Perhaps try N times instead of forever? The patch you've posted uses N
= 1 (which is already better than the existing code) but it can be any
other number.

Roman.

[-- Attachment #2: mkdir-patch-v2.txt --]
[-- Type: text/plain, Size: 929 bytes --]

diff --git a/Src/Modules/files.c b/Src/Modules/files.c
index 6d20e38a8..5a58ad600 100644
--- a/Src/Modules/files.c
+++ b/Src/Modules/files.c
@@ -122,19 +122,29 @@ domkdir(char *nam, char *path, mode_t mode, int p)
 {
     int err;
     mode_t oumask;
+    struct stat st;
+    int n = 8;
     char const *rpath = unmeta(path);
 
-    if(p) {
-	struct stat st;
-
-	if(!stat(rpath, &st) && S_ISDIR(st.st_mode))
+    while(n--) {
+	oumask = umask(0);
+	err = mkdir(rpath, mode) ? errno : 0;
+	umask(oumask);
+	if (!err)
+	    return 0;
+	if(!p || err != EEXIST)
+	    break;
+	if(!stat(rpath, &st)) {
+	    if(errno == ENOENT)
+		continue;
+	    err = errno;
+	    break;
+	}
+	if(S_ISDIR(st.st_mode))
 	    return 0;
+	break;
     }
-    oumask = umask(0);
-    err = mkdir(rpath, mode) ? errno : 0;
-    umask(oumask);
-    if(!err)
-	return 0;
+
     zwarnnam(nam, "cannot make directory `%s': %e", path, err);
     return 1;
 }

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

* Re: [patch] Avoid race in zf_mkdir
  2020-10-10 11:50             ` Roman Perepelitsa
@ 2020-10-15 10:01               ` Roman Perepelitsa
  2020-10-15 15:29                 ` Bart Schaefer
  0 siblings, 1 reply; 14+ messages in thread
From: Roman Perepelitsa @ 2020-10-15 10:01 UTC (permalink / raw)
  To: Zsh hackers list

On Sat, Oct 10, 2020 at 1:50 PM Roman Perepelitsa
<roman.perepelitsa@gmail.com> wrote:
>
> Perhaps try N times instead of forever? The patch you've posted uses N
> = 1 (which is already better than the existing code) but it can be any
> other number.

Does anyone have an opinion on the value of N? My latest patch sets
N=8 but N=1 would be fine in practice, too. I'd like to fix this so
that I can stop using a workaround whenever I invoke `zf_mkdir -p`.

Roman.


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

* Re: [patch] Avoid race in zf_mkdir
  2020-10-15 10:01               ` Roman Perepelitsa
@ 2020-10-15 15:29                 ` Bart Schaefer
  2020-10-15 15:36                   ` Roman Perepelitsa
  0 siblings, 1 reply; 14+ messages in thread
From: Bart Schaefer @ 2020-10-15 15:29 UTC (permalink / raw)
  To: Roman Perepelitsa; +Cc: Zsh hackers list

On Thu, Oct 15, 2020 at 3:02 AM Roman Perepelitsa
<roman.perepelitsa@gmail.com> wrote:
>
> Does anyone have an opinion on the value of N? My latest patch sets
> N=8 but N=1 would be fine in practice, too.

Any value > 1 would probably render the chances of an incorrect result
astronomically small.

Minor nits about the patch:
- zsh coding style prefers a space between keywords such as if/while,
and the open paren.
- I'm paranoid about tests like "while (n--)", I prefer "while (n-- > 0)".


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

* Re: [patch] Avoid race in zf_mkdir
  2020-10-15 15:29                 ` Bart Schaefer
@ 2020-10-15 15:36                   ` Roman Perepelitsa
  2020-10-15 16:47                     ` Bart Schaefer
  2020-10-22 13:30                     ` Roman Perepelitsa
  0 siblings, 2 replies; 14+ messages in thread
From: Roman Perepelitsa @ 2020-10-15 15:36 UTC (permalink / raw)
  To: Bart Schaefer; +Cc: Zsh hackers list

On Thu, Oct 15, 2020 at 5:29 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> On Thu, Oct 15, 2020 at 3:02 AM Roman Perepelitsa
> <roman.perepelitsa@gmail.com> wrote:
> >
> > Does anyone have an opinion on the value of N? My latest patch sets
> > N=8 but N=1 would be fine in practice, too.
>
> Any value > 1 would probably render the chances of an incorrect result
> astronomically small.

Thanks for taking a look. You aren't opposed to N=8, right? I slightly
prefer N > 1 and 8 is the largest round number expressible with a
single character, so :-)

> Minor nits about the patch:
> - zsh coding style prefers a space between keywords such as if/while,
> and the open paren.

I'm following the local style of the file I'm changing:
https://github.com/zsh-users/zsh/blob/master/Src/Modules/files.c

Should I go with the project style in my diff? It would make
formatting within the file inconsistent.

> - I'm paranoid about tests like "while (n--)", I prefer "while (n-- > 0)".

Changed. (Not publishing the new patch yet. Will do so once this
discussion concludes.)

Roman.


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

* Re: [patch] Avoid race in zf_mkdir
  2020-10-15 15:36                   ` Roman Perepelitsa
@ 2020-10-15 16:47                     ` Bart Schaefer
  2020-10-22 13:30                     ` Roman Perepelitsa
  1 sibling, 0 replies; 14+ messages in thread
From: Bart Schaefer @ 2020-10-15 16:47 UTC (permalink / raw)
  To: Roman Perepelitsa; +Cc: Zsh hackers list

On Thu, Oct 15, 2020 at 8:37 AM Roman Perepelitsa
<roman.perepelitsa@gmail.com> wrote:
>
> I'm following the local style of the file I'm changing:
> https://github.com/zsh-users/zsh/blob/master/Src/Modules/files.c

Ah, OK (sigh).  That is the right thing to do.


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

* Re: [patch] Avoid race in zf_mkdir
  2020-10-15 15:36                   ` Roman Perepelitsa
  2020-10-15 16:47                     ` Bart Schaefer
@ 2020-10-22 13:30                     ` Roman Perepelitsa
  1 sibling, 0 replies; 14+ messages in thread
From: Roman Perepelitsa @ 2020-10-22 13:30 UTC (permalink / raw)
  To: Bart Schaefer; +Cc: Zsh hackers list

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

On Thu, Oct 15, 2020 at 5:36 PM Roman Perepelitsa
<roman.perepelitsa@gmail.com> wrote:
>
> On Thu, Oct 15, 2020 at 5:29 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
> >
> > - I'm paranoid about tests like "while (n--)", I prefer "while (n-- > 0)".
>
> Changed. (Not publishing the new patch yet. Will do so once this
> discussion concludes.)

Here's the fixed version. Will commit soon.

Roman.

[-- Attachment #2: mkdir-patch-v3.txt --]
[-- Type: text/plain, Size: 932 bytes --]

diff --git a/Src/Modules/files.c b/Src/Modules/files.c
index 6d20e38a8..7ebacba6c 100644
--- a/Src/Modules/files.c
+++ b/Src/Modules/files.c
@@ -122,19 +122,29 @@ domkdir(char *nam, char *path, mode_t mode, int p)
 {
     int err;
     mode_t oumask;
+    struct stat st;
+    int n = 8;
     char const *rpath = unmeta(path);
 
-    if(p) {
-	struct stat st;
-
-	if(!stat(rpath, &st) && S_ISDIR(st.st_mode))
+    while(n-- > 0) {
+	oumask = umask(0);
+	err = mkdir(rpath, mode) ? errno : 0;
+	umask(oumask);
+	if (!err)
+	    return 0;
+	if(!p || err != EEXIST)
+	    break;
+	if(stat(rpath, &st)) {
+	    if(errno == ENOENT)
+		continue;
+	    err = errno;
+	    break;
+	}
+	if(S_ISDIR(st.st_mode))
 	    return 0;
+	break;
     }
-    oumask = umask(0);
-    err = mkdir(rpath, mode) ? errno : 0;
-    umask(oumask);
-    if(!err)
-	return 0;
+
     zwarnnam(nam, "cannot make directory `%s': %e", path, err);
     return 1;
 }

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

end of thread, other threads:[~2020-10-22 13:31 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-09 20:07 [patch] Avoid race in zf_mkdir Matthew Martin
2020-10-09 20:24 ` Bart Schaefer
2020-10-09 20:35   ` Roman Perepelitsa
2020-10-09 20:47     ` Bart Schaefer
2020-10-09 20:53       ` Matthew Martin
2020-10-09 21:22         ` Roman Perepelitsa
2020-10-09 21:27           ` Bart Schaefer
2020-10-10 11:50             ` Roman Perepelitsa
2020-10-15 10:01               ` Roman Perepelitsa
2020-10-15 15:29                 ` Bart Schaefer
2020-10-15 15:36                   ` Roman Perepelitsa
2020-10-15 16:47                     ` Bart Schaefer
2020-10-22 13:30                     ` Roman Perepelitsa
2020-10-09 21:40           ` Matthew Martin

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).