[FFmpeg-devel] [PATCH] avformat/img2enc: added av_get_frame_filename3() (changed parameter int -> int64_t)

[Filip Mašić]

I just realised image2 has a specific maintainer so I’d like to ask for
guidance for my patch, so I CCed. Its point is to fix an integer overflow
in img2enc, so I was unsure what to name it, or whether it qualifies as a
security fix so sorry if I bumped it prematurely. I’ve looked into it a bit
more, and am curious if it merits making the next release.

The original implementation of the frame_pts option was a shortcut compared
to adding more general parameters to the output filename of image2, like %t
I’ve seen suggested for pts in other issues. It would have to have ignored
a warning for narrowing type conversion, and further has no regression
tests for the feature. I considered making some but couldn’t find any tests
in the suite to mimic, since it would need to be testing filenames rather
than file contents. It would be good to consider either a unit test of the
edited function (the one in the patch name) or a full command-based test,
both on a long-enough video (actual or simulated) that would require int64
for a microsecond frame timestamp, or similar.

I still can’t test it myself; I got past the CRLF Make bug but my compiled
ffmpeg is missing image2 and png and idk what flags and packages I need to
add there or how to.

We would also want to update the API changelog and maybe deprecate the
older versions of the upgraded function.

Thanks for your consideration :).

On Monday 16 September 2024, Filip Mašić <shoutplenty at gmail.com> wrote:

> fix for ticket 11194 (frame_pts codepath requires a larger integer
> capacity for normal workloads)
>
> Signed-off-by: Filip Mašić <shoutplenty at gmail.com>
> ---
>  libavformat/avformat.h |  5 ++++-
>  libavformat/img2enc.c  |  6 ++----
>  libavformat/utils.c    | 12 +++++++++---
>  3 files changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> index 4a3fb00529..8a5cfa0582 100644
> --- a/libavformat/avformat.h
> +++ b/libavformat/avformat.h
> @@ -2906,10 +2906,13 @@ void av_dump_format(AVFormatContext *ic,
>   * @param buf destination buffer
>   * @param buf_size destination buffer size
>   * @param path numbered sequence string
> - * @param number frame number
> + * @param number frame number (or pts if frame_pts option passed)
>   * @param flags AV_FRAME_FILENAME_FLAGS_*
>   * @return 0 if OK, -1 on format error
>   */
> +int av_get_frame_filename3(char *buf, int buf_size,
> +                          const char *path, int64_t number, int flags);
> +
>  int av_get_frame_filename2(char *buf, int buf_size,
>                            const char *path, int number, int flags);
>
> diff --git a/libavformat/img2enc.c b/libavformat/img2enc.c
> index 526a11e5ee..460e6a38bd 100644
> --- a/libavformat/img2enc.c
> +++ b/libavformat/img2enc.c
> @@ -160,13 +160,11 @@ static int write_packet(AVFormatContext *s, AVPacket
> *pkt)
>              return AVERROR(EINVAL);
>          }
>      } else if (img->frame_pts) {
> -        if (av_get_frame_filename2(filename, sizeof(filename), s->url,
> pkt->pts, AV_FRAME_FILENAME_FLAGS_MULTIPLE) < 0) {
> +        if (av_get_frame_filename3(filename, sizeof(filename), s->url,
> pkt->pts, AV_FRAME_FILENAME_FLAGS_MULTIPLE) < 0) {
>              av_log(s, AV_LOG_ERROR, "Cannot write filename by pts of the
> frames.");
>              return AVERROR(EINVAL);
>          }
> -    } else if (av_get_frame_filename2(filename, sizeof(filename), s->url,
> -                                      img->img_number,
> -                                      AV_FRAME_FILENAME_FLAGS_MULTIPLE)
> < 0) {
> +    } else if (av_get_frame_filename3(filename, sizeof(filename),
> s->url, img->img_number, AV_FRAME_FILENAME_FLAGS_MULTIPLE) < 0) {
>          if (img->img_number == img->start_img_number) {
>              av_log(s, AV_LOG_WARNING, "The specified filename '%s' does
> not contain an image sequence pattern or a pattern is invalid.\n", s->url);
>              av_log(s, AV_LOG_WARNING,
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index e9ded627ad..36017805a0 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -19,6 +19,7 @@
>   * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> 02110-1301 USA
>   */
>
> +#include <inttypes.h>
>  #include <stdint.h>
>
>  #include "config.h"
> @@ -280,7 +281,7 @@ uint64_t ff_parse_ntp_time(uint64_t ntp_ts)
>      return (sec * 1000000) + usec;
>  }
>
> -int av_get_frame_filename2(char *buf, int buf_size, const char *path, int
> number, int flags)
> +int av_get_frame_filename3(char *buf, int buf_size, const char *path,
> int64_t number, int flags)
>  {
>      const char *p;
>      char *q, buf1[20], c;
> @@ -313,7 +314,7 @@ int av_get_frame_filename2(char *buf, int buf_size,
> const char *path, int number
>                  percentd_found = 1;
>                  if (number < 0)
>                      nd += 1;
> -                snprintf(buf1, sizeof(buf1), "%0*d", nd, number);
> +                snprintf(buf1, sizeof(buf1), "%0*"PRId64, nd, number);
>                  len = strlen(buf1);
>                  if ((q - buf + len) > buf_size - 1)
>                      goto fail;
> @@ -338,9 +339,14 @@ fail:
>      return -1;
>  }
>
> +int av_get_frame_filename2(char *buf, int buf_size, const char *path, int
> number, int flags)
> +{
> +    return av_get_frame_filename3(buf, buf_size, path, (int64_t)number,
> flags);
> +}
> +
>  int av_get_frame_filename(char *buf, int buf_size, const char *path, int
> number)
>  {
> -    return av_get_frame_filename2(buf, buf_size, path, number, 0);
> +    return av_get_frame_filename3(buf, buf_size, path, (int64_t)number,
> 0);
>  }
>
>  void av_url_split(char *proto, int proto_size,
> --
> 2.46.0.windows.1
>
>