[FFmpeg-devel] [PATCH 1/2] Add av_fd_dump(): read from fd into buffer up to EOF

[Nicolas George]

Le sextidi 6 messidor, an CCXXII, Andrey Utkin a écrit :
> ---
>  libavutil/file.c | 32 ++++++++++++++++++++++++++++++++
>  libavutil/file.h | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 62 insertions(+)
> 
> diff --git a/libavutil/file.c b/libavutil/file.c
> index 45fe853..1c664f2 100644
> --- a/libavutil/file.c
> +++ b/libavutil/file.c
> @@ -184,6 +184,38 @@ int av_tempfile(const char *prefix, char **filename, int log_offset, void *log_c
>      return fd; /* success */
>  }
>  
> +int av_fd_dump(int fd, uint8_t **bufptr, size_t *size,
> +        size_t spare_trailing_bytes, size_t buf_incr)
> +{
> +    int ret;
> +    int should_free_on_failure;
> +    int bufsize;

> +    assert(bufptr && size);

See Michael's reply. I believe this API does not make any sense without
bufptr and size, therefore I believe an av_assert is best.

> +    should_free_on_failure = (*bufptr == NULL);
> +    /* We don't pass explicitly current allocated size, but it is safe to
> +     * assume it equal to the offset to start, i.e. *size */
> +    bufsize = *size;
> +    while (1) {
> +        if (bufsize - *size <= spare_trailing_bytes) {

> +            bufsize += buf_incr;

Possible integer overflow.

Plus, of course, the fact already pointed that using a constant increment is
inefficient: it makes reading the file quadratic. Probably not a big issue
for files that we want to read directly into memory, but still.

The usual practice is to double the buffer each time. We already have
several APIs to do that. For example, you could replace all this by
something like that:

int av_bprint_fd_contents(AVBPrint *bp, int fd);

> +            ret = av_reallocp(bufptr, bufsize);
> +            if (ret)
> +                break;
> +        }

> +        ret = read(fd, *bufptr + *size, bufsize - *size - spare_trailing_bytes);

If buf_incr is greater than INT_MAX, ret may overflow. Unlikely but
possible. read(2) return value is ssize_t.

> +        if (!ret) {
> +            break;

> +        } else if (ret == -1) {

I believe "ret < 0" should be slightly more robust against slightly
non-standard systems.

> +            ret = AVERROR(errno);
> +            break;
> +        }
> +        *size += ret;
> +    }
> +    if (ret && should_free_on_failure)
> +        av_reallocp(*bufptr, 0);
> +    return ret;
> +}
> +
>  #ifdef TEST
>  
>  #undef printf
> diff --git a/libavutil/file.h b/libavutil/file.h
> index a7364fe..db585d4 100644
> --- a/libavutil/file.h
> +++ b/libavutil/file.h
> @@ -63,4 +63,34 @@ void av_file_unmap(uint8_t *bufptr, size_t size);
>   */
>  int av_tempfile(const char *prefix, char **filename, int log_offset, void *log_ctx);
>  
> +/**
> + * Reads from fd up to EOF, saves data in *bufptr and updates *size.
> + *

> + * @note You MUST initialize *bufptr and *size to valid values. If you set them

Nit: impersonal forms seem more natural in documentation: "*bufptr and *size
MUST be initialized to valid values..."

> + * to NULL and 0, it will allocate the array for you and fill it with fd
> + * product. Otherwise you can set it to an address of some previously
> + * av_realloc()-ed buffer and the offset to proceed writing, and it will
> + * effectively append fd product to existing content of array. Array will get
> + * extended by av_reallocp() if needed.
> + *
> + * @note According to av_realloc() doc, it may work wrong if supplied with
> + * av_malloc()-ed buffer. Use av_realloc() to create pre-allocated buffer.
> + *
> + * @note fd is not closed by this function
> + *
> + * @param fd file descriptor to read from
> + * @param bufptr pointer to pointer to buffer, it must be initialized by caller.
> + * @param size pointer to amount of data written to buffer, it must be
> + * initialized by caller.
> + * @param spare_trailing_bytes how many allocated bytes to hold for later
> + * buffer finalization. Set to 0 if not needed, set to 1 if you need to
> + * null-terminate the buffer.
> + * @param buf_incr amount of bytes to increase the buffer size when needed
> + *
> + * @return zero on success, negative error code on failure
> + *
> + */

> +int av_fd_dump(int fd, uint8_t **bufptr, size_t *size,
> +        size_t spare_trailing_bytes, size_t buf_incr);

Nit: I do not like the name very much. Maybe simply
"af_file_read_from_fd()"?

As a side note, av_file_map() uses a file name while this function uses an
Unix fd. Ideally, all these operations should be possible on any AVIO-style
object, but that would mean moving this from lavu to lavf. That is the price
of over-splitting libraries.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140626/2429cbe0/attachment.asc>