[prev in list] [next in list] [prev in thread] [next in thread] 

List:       busybox
Subject:    Re: [PATCH] blkdiscard: check that the file is a block device before opening
From:       Ari Sundholm <ari () tuxera ! com>
Date:       2016-01-05 12:19:08
Message-ID: 1451996348.16365.33.camel () ari-lenovo
[Download RAW message or body]

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
ari@tuxera.com
> 
> On Tue, Jan 5, 2016 at 11:50 AM, Ari Sundholm <ari@tuxera.com> 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 <ari@tuxera.com>
>         ---
>          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
>         busybox@busybox.net
>         http://lists.busybox.net/mailman/listinfo/busybox
> 
> 


_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[prev in list] [next in list] [prev in thread] [next in thread]