Hi! Thanks for your comments.
On Tue, 2016-01-05 at 11:54 +0000, Sam Liddicott wrote: > This patch just trades one undefined behaviour for another. I'd rather think of it as reducing the number of cases where we would get undefined behavior, while not completely eliminating them. > > I'd open first and then do fstat. > Otherwise change your comment, because it isn't really enforcing > anything with a race condition between the stat and the open. Usually, I would agree with you. However, in this case doing the stat() before the open() was the entire point. While it is true that a race would remain, the patch is strictly an improvement, as the cases of getting undefined open() behavior are reduced to the race, instead of hitting it every time a non-block device file is given. I am fully aware this is not ideal and welcome any suggestions how to do this more cleanly. Your point about the comment is valid, though: strictly speaking, the check does not enforce the block deviceness due to the race but only does a best-effort check. I will change the comment. > Sam Best regards, Ari Sundholm [email protected] > > On Tue, Jan 5, 2016 at 11:50 AM, Ari Sundholm <[email protected]> wrote: > We need to enforce that the opened file is a block device, as > opening a file with the O_EXCL flag on and the O_CREATE flag > off has > undefined behavior unless the file is a block device. > > Signed-off-by: Ari Sundholm <[email protected]> > --- > util-linux/blkdiscard.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/util-linux/blkdiscard.c b/util-linux/blkdiscard.c > index ace88a1..1b234fa 100644 > --- a/util-linux/blkdiscard.c > +++ b/util-linux/blkdiscard.c > @@ -38,7 +38,7 @@ int blkdiscard_main(int argc UNUSED_PARAM, > char **argv) > uint64_t offset; /* Leaving these two variables out > does not */ > uint64_t length; /* shrink code size and hampers > readability. */ > uint64_t range[2]; > -// struct stat st; > + struct stat st; > int fd; > > enum { > @@ -51,11 +51,15 @@ int blkdiscard_main(int argc UNUSED_PARAM, > char **argv) > opts = getopt32(argv, "o:l:s", &offset_str, > &length_str); > argv += optind; > > + /* We need to enforce that the opened file is a block > device, as > + * opening a file with the O_EXCL flag on and the > O_CREATE flag off has > + * undefined behavior unless the file is a block > device. > + */ > + xstat(argv[0], &st); > + if (!S_ISBLK(st.st_mode)) > + bb_error_msg_and_die("%s: not a block device", > argv[0]); > + > fd = xopen(argv[0], O_RDWR|O_EXCL); > -//Why bother, BLK[SEC]DISCARD will fail on non-blockdevs > anyway? > -// xfstat(fd, &st); > -// if (!S_ISBLK(st.st_mode)) > -// bb_error_msg_and_die("%s: not a block device", > argv[0]); > > offset = xatoull_sfx(offset_str, kMG_suffixes); > > -- > 1.9.1 > > > > _______________________________________________ > busybox mailing list > [email protected] > http://lists.busybox.net/mailman/listinfo/busybox > > _______________________________________________ busybox mailing list [email protected] http://lists.busybox.net/mailman/listinfo/busybox
