diff mbox

[FFmpeg-devel,3/3] lavf/concat: implement FFSEEK_SIZE.

Message ID 20190719122354.32460-3-george@nsup.org
State Accepted
Commit 3add65e052cf3511b284d53e73f986a6768e5486
Headers show

Commit Message

Nicolas George July 19, 2019, 12:23 p.m. UTC
Signed-off-by: Nicolas George <george@nsup.org>
---
 libavformat/concat.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Andreas Rheinhardt July 19, 2019, 4:27 p.m. UTC | #1
The commit title is wrong: It's AVSEEK_SIZE.

Nicolas George:
> Signed-off-by: Nicolas George <george@nsup.org>
> ---
>  libavformat/concat.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/concat.c b/libavformat/concat.c
> index 19c83c309a..ea3bc1dfde 100644
> --- a/libavformat/concat.c
> +++ b/libavformat/concat.c
> @@ -38,6 +38,7 @@ struct concat_data {
>      struct concat_nodes *nodes;    ///< list of nodes to concat
>      size_t               length;   ///< number of cat'ed nodes
>      size_t               current;  ///< index of currently read node
> +    uint64_t total_size;
>  };
>  
>  static av_cold int concat_close(URLContext *h)
> @@ -59,7 +60,7 @@ static av_cold int concat_open(URLContext *h, const char *uri, int flags)
>  {
>      char *node_uri = NULL;
>      int err = 0;
> -    int64_t size;
> +    int64_t size, total_size = 0;
>      size_t len, i;
>      URLContext *uc;
>      struct concat_data  *data = h->priv_data;
> @@ -112,6 +113,7 @@ static av_cold int concat_open(URLContext *h, const char *uri, int flags)
>          /* assembling */
>          nodes[i].uc   = uc;
>          nodes[i].size = size;
> +        total_size += size;
There is a potential for overflow here which is undefined behaviour as
total_size is signed. Simply making it unsigned (like you store it)
will solve the undefined behaviour here, but if a wraparound happened,
then seeking may return a nonsense value (not only when using
AVSEEK_SIZE, but certainly when using AVSEEK_SIZE) and may not work at
all. I see two solutions for this:
a) Erroring out when the total size doesn't fit into a int64_t.
b) Disallow seeking if the total size doesn't fit into a int64_t (in
this case, one might even allow input files that don't report a usable
size).
I don't know if a) would break any use cases (maybe some (buggy)
protocol reports INT64_MAX if the size is unknown?).
>      }
>      av_free(node_uri);
>      data->length = i;
> @@ -123,6 +125,7 @@ static av_cold int concat_open(URLContext *h, const char *uri, int flags)
>          err = AVERROR(ENOMEM);
>      } else
>          data->nodes = nodes;
> +    data->total_size = total_size;
>      return err;
>  }
>  
> @@ -158,6 +161,8 @@ static int64_t concat_seek(URLContext *h, int64_t pos, int whence)
>      struct concat_nodes *nodes = data->nodes;
>      size_t i;
>  
> +    if ((whence & AVSEEK_SIZE))
Unnecessary parentheses.
> +        return data->total_size;
>      switch (whence) {
>      case SEEK_END:
>          for (i = data->length - 1; i && pos < -nodes[i].size; i--)
>
Nicolas George July 19, 2019, 4:38 p.m. UTC | #2
Andreas Rheinhardt (12019-07-19):
> The commit title is wrong: It's AVSEEK_SIZE.

Locally fixed, thanks.

Regards,
diff mbox

Patch

diff --git a/libavformat/concat.c b/libavformat/concat.c
index 19c83c309a..ea3bc1dfde 100644
--- a/libavformat/concat.c
+++ b/libavformat/concat.c
@@ -38,6 +38,7 @@  struct concat_data {
     struct concat_nodes *nodes;    ///< list of nodes to concat
     size_t               length;   ///< number of cat'ed nodes
     size_t               current;  ///< index of currently read node
+    uint64_t total_size;
 };
 
 static av_cold int concat_close(URLContext *h)
@@ -59,7 +60,7 @@  static av_cold int concat_open(URLContext *h, const char *uri, int flags)
 {
     char *node_uri = NULL;
     int err = 0;
-    int64_t size;
+    int64_t size, total_size = 0;
     size_t len, i;
     URLContext *uc;
     struct concat_data  *data = h->priv_data;
@@ -112,6 +113,7 @@  static av_cold int concat_open(URLContext *h, const char *uri, int flags)
         /* assembling */
         nodes[i].uc   = uc;
         nodes[i].size = size;
+        total_size += size;
     }
     av_free(node_uri);
     data->length = i;
@@ -123,6 +125,7 @@  static av_cold int concat_open(URLContext *h, const char *uri, int flags)
         err = AVERROR(ENOMEM);
     } else
         data->nodes = nodes;
+    data->total_size = total_size;
     return err;
 }
 
@@ -158,6 +161,8 @@  static int64_t concat_seek(URLContext *h, int64_t pos, int whence)
     struct concat_nodes *nodes = data->nodes;
     size_t i;
 
+    if ((whence & AVSEEK_SIZE))
+        return data->total_size;
     switch (whence) {
     case SEEK_END:
         for (i = data->length - 1; i && pos < -nodes[i].size; i--)