* Bug? "fstat64(f, &sb) < 0 && S_ISREG(sb.st_mode)"
@ 2016-10-25 15:33 Denys Vlasenko
2016-10-25 17:13 ` Eduardo Bustamante
0 siblings, 1 reply; 5+ messages in thread
From: Denys Vlasenko @ 2016-10-25 15:33 UTC (permalink / raw)
To: Herbert Xu, dash
/* Take care of noclobber mode. */
if (Cflag) {
fname = redir->nfile.expfname;
if (stat64(fname, &sb) < 0) {
if ((f = open64(fname,
O_WRONLY|O_CREAT|O_EXCL, 0666)) < 0)
goto ecreate;
} else if (!S_ISREG(sb.st_mode)) {
if ((f = open64(fname, O_WRONLY, 0666)) < 0)
goto ecreate;
if (fstat64(f, &sb) < 0 &&
S_ISREG(sb.st_mode)) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
was it meant to be "fstat64(f, &sb) < 0 || S_ISREG(sb.st_mode)" ?
close(f);
errno = EEXIST;
goto ecreate;
}
} else {
errno = EEXIST;
goto ecreate;
}
break;
}
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Bug? "fstat64(f, &sb) < 0 && S_ISREG(sb.st_mode)"
2016-10-25 15:33 Bug? "fstat64(f, &sb) < 0 && S_ISREG(sb.st_mode)" Denys Vlasenko
@ 2016-10-25 17:13 ` Eduardo Bustamante
2016-10-25 17:19 ` Eduardo Bustamante
2016-10-26 21:43 ` Jilles Tjoelker
0 siblings, 2 replies; 5+ messages in thread
From: Eduardo Bustamante @ 2016-10-25 17:13 UTC (permalink / raw)
To: Denys Vlasenko; +Cc: Herbert Xu, dash
I see the change was introduced here
http://git.kernel.org/cgit/utils/dash/dash.git/commit/?id=f78674ed6f95b594dcac0e96d6a76c5f64aa2cbf,
by importing code from FreeBSD's sh.
What I find interesting is testing the contents of 'sb', when the
fstat64 call failed. FreeBSD's code has the opposite:
https://github.com/freebsd/freebsd/blob/master/bin/sh/redir.c#L200,
i.e. they do:
if (fstat(f, &sb) != -1 && S_ISREG(sb.st_mode)) {
BTW, that code was introduced here:
https://github.com/freebsd/freebsd/commit/e3cb9d1015e8283f4f9d116cf50f510741f8c0dd
Also, the way the open is performed has an interesting consequence,
i.e. the redirection operation on a FIFO blocks until something is
written to the FIFO:
hp% dash -Cc 'rm ff; mkfifo ff; echo a > ff'
^Cdash: 1: cannot create ff: Interrupted system call
It seems to be the same case for mksh, ksh93, posh, pdksh. Not that it
matters though.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Bug? "fstat64(f, &sb) < 0 && S_ISREG(sb.st_mode)"
2016-10-25 17:13 ` Eduardo Bustamante
@ 2016-10-25 17:19 ` Eduardo Bustamante
2016-10-26 21:43 ` Jilles Tjoelker
1 sibling, 0 replies; 5+ messages in thread
From: Eduardo Bustamante @ 2016-10-25 17:19 UTC (permalink / raw)
To: Denys Vlasenko; +Cc: Herbert Xu, dash
Also, inspecting bash code, this seems to be code to figure out if the
file was changed from a non-regular file to a regular file between the
stat and the open, so I'd say it's indeed a bug, but the problem is
not the &&, but testing the wrong return code.
From bash redir.c
631 /* OK, the open succeeded, but the file may have been changed from a
632 non-regular file to a regular file between the stat and the open.
633 We are assuming that the O_EXCL open handles the case where FILENAME
634 did not exist and is symlinked to an existing file between the stat
635 and open. */
636
637 /* If we can open it and fstat the file descriptor, and neither check
638 revealed that it was a regular file, and the file has not
been replaced,
639 return the file descriptor. */
640 if ((fstat (fd, &finfo2) == 0) && (S_ISREG (finfo2.st_mode) == 0) &&
641 r == 0 && (S_ISREG (finfo.st_mode) == 0) &&
642 same_file (filename, filename, &finfo, &finfo2))
643 return fd;
644
645 /* The file has been replaced. badness. */
646 close (fd);
647 errno = EEXIST;
648 return (NOCLOBBER_REDIRECT);
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Bug? "fstat64(f, &sb) < 0 && S_ISREG(sb.st_mode)"
2016-10-25 17:13 ` Eduardo Bustamante
2016-10-25 17:19 ` Eduardo Bustamante
@ 2016-10-26 21:43 ` Jilles Tjoelker
2016-10-27 0:35 ` Eduardo Bustamante
1 sibling, 1 reply; 5+ messages in thread
From: Jilles Tjoelker @ 2016-10-26 21:43 UTC (permalink / raw)
To: Eduardo Bustamante; +Cc: Denys Vlasenko, Herbert Xu, dash
On Tue, Oct 25, 2016 at 12:13:51PM -0500, Eduardo Bustamante wrote:
> I see the change was introduced here
> http://git.kernel.org/cgit/utils/dash/dash.git/commit/?id=f78674ed6f95b594dcac0e96d6a76c5f64aa2cbf,
> by importing code from FreeBSD's sh.
> What I find interesting is testing the contents of 'sb', when the
> fstat64 call failed. FreeBSD's code has the opposite:
> https://github.com/freebsd/freebsd/blob/master/bin/sh/redir.c#L200,
> i.e. they do:
> if (fstat(f, &sb) != -1 && S_ISREG(sb.st_mode)) {
> BTW, that code was introduced here:
> https://github.com/freebsd/freebsd/commit/e3cb9d1015e8283f4f9d116cf50f510741f8c0dd
The FreeBSD code is how it was intended to be. I don't know how the dash
bug was introduced. The bug only appears when a parallel actor replaces
an openable non-regular file (such as a symlink to /dev/null) with a
regular file and may cause corruption of the file.
As noted on austin-group-l recently, the code does suffer from an issue
where a noclobber open may fail if a parallel actor deletes a
non-regular file (stat reports non-regular file, open fails). Many other
shells have this issue, though.
> Also, the way the open is performed has an interesting consequence,
> i.e. the redirection operation on a FIFO blocks until something is
> written to the FIFO:
> hp% dash -Cc 'rm ff; mkfifo ff; echo a > ff'
> ^Cdash: 1: cannot create ff: Interrupted system call
> It seems to be the same case for mksh, ksh93, posh, pdksh. Not that it
> matters though.
Blocking is intended behaviour when opening a fifo without a
counterparty. It can be used to good effect for synchronizing processes,
sometimes even without transferring any data.
--
Jilles Tjoelker
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Bug? "fstat64(f, &sb) < 0 && S_ISREG(sb.st_mode)"
2016-10-26 21:43 ` Jilles Tjoelker
@ 2016-10-27 0:35 ` Eduardo Bustamante
0 siblings, 0 replies; 5+ messages in thread
From: Eduardo Bustamante @ 2016-10-27 0:35 UTC (permalink / raw)
To: Jilles Tjoelker; +Cc: Denys Vlasenko, Herbert Xu, dash
>> hp% dash -Cc 'rm ff; mkfifo ff; echo a > ff'
>> ^Cdash: 1: cannot create ff: Interrupted system call
>
>> It seems to be the same case for mksh, ksh93, posh, pdksh. Not that it
>> matters though.
>
> Blocking is intended behaviour when opening a fifo without a
> counterparty. It can be used to good effect for synchronizing processes,
> sometimes even without transferring any data.
Yup, sorry about the noise there. That was just a misunderstanding on my part.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-10-27 0:36 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-25 15:33 Bug? "fstat64(f, &sb) < 0 && S_ISREG(sb.st_mode)" Denys Vlasenko
2016-10-25 17:13 ` Eduardo Bustamante
2016-10-25 17:19 ` Eduardo Bustamante
2016-10-26 21:43 ` Jilles Tjoelker
2016-10-27 0:35 ` Eduardo Bustamante
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).