Re: [PATCH] read-file: Avoid memory reallocations with seekable files.

[Bruno Haible]

Eric Blake wrote:
> I see a few changes to squash in:

I see some more changes to squash in:

> +#include <sys/stat.h>

This requires the use of module 'sys_stat'. (See in
doc/posix-headers/sys_stat.texi; we use S_REG.)

> +/* Get SIZE_MAX.  */
> +#include <stdint.h>

This requires the use of module 'stdint'.

>  /* Get realloc, free. */
>  #include <stdlib.h>

This is now also used to get 'malloc'.

> @@ -38,6 +49,33 @@ fread_file (FILE * stream, size_t * length)
>    size_t alloc = 0;
>    size_t size = 0;
>    int save_errno;
> +  struct stat st;

The number of variables declared at the beginning of this function.
As a general rule, try to minimize the scope of local variables.

> +  do
> +    {
> +      off_t alloc_off, pos;
> +
> +      if (fstat (fileno (stream), &st) < 0 || !S_ISREG (st.st_mode))
> +        break;
> +
> +      pos = ftello (stream);
> +      if (pos < 0 || pos > st.st_size)
> +        break;
> +
> +      alloc_off = st.st_size - pos;
> +      if (SIZE_MAX <= alloc_off)
> +        {
> +          errno = ENOMEM;
> +          return NULL;
> +        }
> +
> +      alloc = alloc_off + 1;
> +
> +      buf = malloc (alloc);
> +      if (!buf)
> +        return NULL;
> +    }
> +  while (0);

The control flow is obfuscated by using a do { } while (0) and several 'break'
statements that act as 'goto' statements. It would be much clearer with 'if'
statements.

>        if (size + BUFSIZ + 1 > alloc)
>          {
>            char *new_buf;
> +          size_t new_alloc = alloc + alloc / 2;
> +
> +          if (new_alloc < alloc)
> +            {
> +              save_errno = ENOMEM;
> +              break;
> +            }

I see not a single comment here. Please put a comment every 10 lines on
average.

I'm committing this combined patch.


2010-08-28  Giuseppe Scrivano  <address@hidden>
            Eric Blake  <address@hidden>
            Bruno Haible  <address@hidden>

        read-file: Avoid memory reallocations with regular files.
        * lib/read-file.c: Include <sys/stat.h>, <stdio.h>, <stdint.h>.
        (fread_file): With regular files, use the remaining length as the
        initial buffer size.  Check against overflow.
        * modules/read-file (Depends-on): Add ftello, malloc-posix, stdint,
        sys_stat.

*** lib/read-file.c.orig        Sat Aug 28 16:11:04 2010
--- lib/read-file.c     Sat Aug 28 16:10:39 2010
***************
*** 20,26 ****
  
  #include "read-file.h"
  
! /* Get realloc, free. */
  #include <stdlib.h>
  
  /* Get errno. */
--- 20,35 ----
  
  #include "read-file.h"
  
! /* Get fstat.  */
! #include <sys/stat.h>
! 
! /* Get ftello.  */
! #include <stdio.h>
! 
! /* Get SIZE_MAX.  */
! #include <stdint.h>
! 
! /* Get malloc, realloc, free. */
  #include <stdlib.h>
  
  /* Get errno. */
***************
*** 36,85 ****
  {
    char *buf = NULL;
    size_t alloc = 0;
-   size_t size = 0;
-   int save_errno;
  
!   for (;;)
!     {
!       size_t count;
!       size_t requested;
! 
!       if (size + BUFSIZ + 1 > alloc)
!         {
!           char *new_buf;
! 
!           alloc += alloc / 2;
!           if (alloc < size + BUFSIZ + 1)
!             alloc = size + BUFSIZ + 1;
! 
!           new_buf = realloc (buf, alloc);
!           if (!new_buf)
!             {
!               save_errno = errno;
                break;
!             }
! 
!           buf = new_buf;
!         }
! 
!       requested = alloc - size - 1;
!       count = fread (buf + size, 1, requested, stream);
!       size += count;
! 
!       if (count != requested)
!         {
!           save_errno = errno;
!           if (ferror (stream))
!             break;
!           buf[size] = '\0';
!           *length = size;
!           return buf;
!         }
!     }
! 
!   free (buf);
!   errno = save_errno;
!   return NULL;
  }
  
  static char *
--- 45,134 ----
  {
    char *buf = NULL;
    size_t alloc = 0;
  
!   /* For a regular file, allocate a buffer that has exactly the right
!      size.  This avoids the need to do dynamic reallocations later.  */
!   {
!     struct stat st;
! 
!     if (fstat (fileno (stream), &st) >= 0 && S_ISREG (st.st_mode))
!       {
!         off_t pos = ftello (stream);
! 
!         if (pos >= 0 && pos < st.st_size)
!           {
!             off_t alloc_off = st.st_size - pos;
! 
!             if (SIZE_MAX <= alloc_off)
!               {
!                 errno = ENOMEM;
!                 return NULL;
!               }
! 
!             alloc = alloc_off + 1;
! 
!             buf = malloc (alloc);
!             if (!buf)
!               /* errno is ENOMEM.  */
!               return NULL;
!           }
!       }
!   }
! 
!   {
!     size_t size = 0; /* number of bytes read so far */
!     int save_errno;
! 
!     for (;;)
!       {
!         size_t count;
!         size_t requested;
! 
!         if (size + BUFSIZ + 1 > alloc)
!           {
!             char *new_buf;
!             size_t new_alloc = alloc + alloc / 2;
! 
!             /* Check against overflow.  */
!             if (new_alloc < alloc)
!               {
!                 save_errno = ENOMEM;
!                 break;
!               }
! 
!             alloc = new_alloc;
!             if (alloc < size + BUFSIZ + 1)
!               alloc = size + BUFSIZ + 1;
! 
!             new_buf = realloc (buf, alloc);
!             if (!new_buf)
!               {
!                 save_errno = errno;
!                 break;
!               }
! 
!             buf = new_buf;
!           }
! 
!         requested = alloc - size - 1;
!         count = fread (buf + size, 1, requested, stream);
!         size += count;
! 
!         if (count != requested)
!           {
!             save_errno = errno;
!             if (ferror (stream))
                break;
!             buf[size] = '\0';
!             *length = size;
!             return buf;
!           }
!       }
! 
!     free (buf);
!     errno = save_errno;
!     return NULL;
!   }
  }
  
  static char *
*** modules/read-file.orig      Sat Aug 28 16:11:04 2010
--- modules/read-file   Sat Aug 28 15:47:24 2010
***************
*** 7,13 ****
--- 7,17 ----
  m4/read-file.m4
  
  Depends-on:
+ ftello
+ malloc-posix
  realloc-posix
+ stdint
+ sys_stat
  
  configure.ac:
  gl_FUNC_READ_FILE