bug#18316: [PATCH] warn on too large file copies

[Pádraig Brady]

On 08/22/2014 08:11 PM, Paul Eggert wrote:
> From 46418ecec04ca61647d6750a0a0fe862f0ae69c1 Mon Sep 17 00:00:00 2001
> From: Paul Eggert <address@hidden>
> Date: Fri, 22 Aug 2014 12:07:11 -0700
> Subject: [PATCH] maint: avoid int64_t and similar types unless they're needed
> 
> C11 doesn't require them, even POSIX doesn't strictly require the
> 64-bit versions, and it makes the code a bit clearer if they're
> used only when needed.
> * src/copy.c (write_zeros, extent_copy):
> * src/extent-scan.h (struct extent_info.ext_length):
> Use off_t, not uint64_t, for a value derived from a file offset.
> * src/extent-scan.h (struct extent_info.ext_flags)
> Prefer plain unsigned int to uint32_t where either will do.
> (struct extent_scan.ei_count):
> Use size_t, not uint32_t, for a value bounded by SIZE_MAX.
> * src/factor.c (MAGIC64, MAGIC63, MAGIC65):
> Remove unnecessary casts to uint64_t.
> ---
>  src/copy.c        | 10 +++++-----
>  src/extent-scan.h |  8 ++++----
>  src/factor.c      |  6 +++---
>  3 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/src/copy.c b/src/copy.c
> index 26d5bdd..a9561c6 100644
> --- a/src/copy.c
> +++ b/src/copy.c
> @@ -251,7 +251,7 @@ clone_file (int dest_fd, int src_fd)
>  /* Write N_BYTES zero bytes to file descriptor FD.  Return true if 
> successful.
>     Upon write failure, set errno and return false.  */
>  static bool
> -write_zeros (int fd, uint64_t n_bytes)
> +write_zeros (int fd, off_t n_bytes)
>  {
>    static char *zeros;
>    static size_t nz = IO_BUFSIZE;
> @@ -272,7 +272,7 @@ write_zeros (int fd, uint64_t n_bytes)
>  
>    while (n_bytes)
>      {
> -      uint64_t n = MIN (nz, n_bytes);
> +      size_t n = MIN (nz, n_bytes);
>        if ((full_write (fd, zeros, n)) != n)
>          return false;
>        n_bytes -= n;

The above are fine and good.

> @@ -296,7 +296,7 @@ extent_copy (int src_fd, int dest_fd, char *buf, size_t 
> buf_size,
>  {
>    struct extent_scan scan;
>    off_t last_ext_start = 0;
> -  uint64_t last_ext_len = 0;
> +  off_t last_ext_len = 0;
>  
>    /* Keep track of the output position.
>       We may need this at the end, for a final ftruncate.  */
> @@ -330,8 +330,8 @@ extent_copy (int src_fd, int dest_fd, char *buf, size_t 
> buf_size,
>        for (i = 0; i < scan.ei_count || empty_extent; i++)
>          {
>            off_t ext_start;
> -          uint64_t ext_len;
> -          uint64_t hole_size;
> +          off_t ext_len;
> +          off_t hole_size;
>
>            if (i < scan.ei_count)
>              {
> diff --git a/src/extent-scan.h b/src/extent-scan.h
> index fa80034..73202a9 100644
> --- a/src/extent-scan.h
> +++ b/src/extent-scan.h
> @@ -26,10 +26,10 @@ struct extent_info
>    off_t ext_logical;
>  
>    /* Extent length.  */
> -  uint64_t ext_length;
> +  off_t ext_length;

The off_t changes assume we compile with -D_FILE_OFFSET_BITS=64 on 32 bit,
which we do, but it's a bit coupled.

>  
>    /* Extent flags, use it for FIEMAP only, or set it to zero.  */
> -  uint32_t ext_flags;
> +  unsigned int ext_flags;
>  };
>  
>  /* Structure used to reserve extent scan information per file.  */
> @@ -42,10 +42,10 @@ struct extent_scan
>    off_t scan_start;
>  
>    /* Flags to use for scan.  */
> -  uint32_t fm_flags;
> +  unsigned int fm_flags;

We check a few low bit flags which is OK though a bit coupled,
and we also merge extents if the flags are the same which could
cause invalid merging on 32 bit if the flags ever expanded to 64
bits which they could theoretically do as there is reserved space for
that in the fiemap structures.  I suppose we could protect against
that unlikely possibility with:

diff --git a/src/extent-scan.c b/src/extent-scan.c
index 805997a..3af8f99 100644
--- a/src/extent-scan.c
+++ b/src/extent-scan.c
@@ -140,6 +140,8 @@ extent_scan_read (struct extent_scan *scan)
           assert (fm_extents[i].fe_logical
                   <= OFF_T_MAX - fm_extents[i].fe_length);

+          verify (sizeof last_ei->ext_flags >= sizeof fm_extents->fe_flags);
+
           if (si && last_ei->ext_flags
               == (fm_extents[i].fe_flags & ~FIEMAP_EXTENT_LAST)
               && (last_ei->ext_logical + last_ei->ext_length

>    /* How many extent info returned for a scan.  */
> -  uint32_t ei_count;
> +  size_t ei_count;

We verify ei_count within SIZE_MAX anyway so this is good.

>    /* If true, fall back to a normal copy, either set by the
>       failure of ioctl(2) for FIEMAP or lseek(2) with SEEK_DATA.  */
> diff --git a/src/factor.c b/src/factor.c
> index 63924d5..f7beaeb 100644
> --- a/src/factor.c
> +++ b/src/factor.c
> @@ -1811,9 +1811,9 @@ isqrt2 (uintmax_t nh, uintmax_t nl)
>  }
>  
>  /* MAGIC[N] has a bit i set iff i is a quadratic residue mod N. */
> -#define MAGIC64 ((uint64_t) 0x0202021202030213ULL)
> -#define MAGIC63 ((uint64_t) 0x0402483012450293ULL)
> -#define MAGIC65 ((uint64_t) 0x218a019866014613ULL)
> +#define MAGIC64 0x0202021202030213ULL
> +#define MAGIC63 0x0402483012450293ULL
> +#define MAGIC65 0x218a019866014613ULL

+1

In summary all the changes look good,
though I might push the above amendment for defensive reasons.

cheers,
Pádraig.