[PATCH] blkdiscard: check that the file is a block device before opening

Ari Sundholm ari at tuxera.com
Tue Jan 5 12:19:08 UTC 2016


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




More information about the busybox mailing list