Re: [FFmpeg-devel] [PATCH]lavf/concatdec: Allocate the filenames on the heap

[Nicolas George]

> 2017-10-21 18:29 GMT+02:00 Nicolas George :
> > Yes, exactly. This limitation was the reason I did not bother handling
> > longer lines. I would like to understand how it makes a difference.

The answer is: the filename field in AVFormatContext is not used to open
the file, only the argument to avformat_open_input().

Le decadi 30 vendémiaire, an CCXXVI, Carl Eugen Hoyos a écrit :
> New patch attached, please comment.

It is the same patch, with the memory leak fixed.

What Marton was saying, and I agree with him, is that 50002 is not less
arbitrary than 4096. It seems quite more so, in fact.

You cannot pretend you fixed the issue if you just raised the limit. I
do not oppose this patch per se, but you cannot close ticket #6761,
because if you do so, anybody will provide a concat file that requires
50003 and reopen.

Regards,

-- 
  Nicolas George


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH]lavf/concatdec: Allocate the filenames on the heap

[Marton Balint]

On Sat, 21 Oct 2017, Carl Eugen Hoyos wrote:


2017-10-21 18:29 GMT+02:00 Nicolas George :

Le decadi 30 vendémiaire, an CCXXVI, Marton Balint a écrit :

I thought filenames in libavformat are limited to 1K anyway
because of the AVFormatContext->filename field.


How can I reproduce this?
I tested the data from the ticket from the command line
and it worked fine here.

Or is there maybe no limitation for the data protocol?



Okay, I checked the code, and libavformat opens the file with the original 
filename, not the truncated one which is stored in AVFormatContext, so the 
1024 character file name limit does not affect every use case.



So if your patch really fixes the ticket, then how? :)


So the original (and the attached) patch do not fix the
ticket for you?
Could you provide some console output to allow me to
better understand the issue?


You cannot open for example an image sequence which has a path longer 
than 1024 bytes.





Yes, exactly. This limitation was the reason I did not bother handling
longer lines. I would like to understand how it makes a difference.


New patch attached, please comment.


Like I said earlier, with only a little additional work you can eliminate 
the concat file name limit if you if you create a line reader function in 
avio which dynamically extends the available buffer, I think that is the 
better solution, because next time somebody will want 100k and not 50k.


Regards,
Marton
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH]lavf/concatdec: Allocate the filenames on the heap

[Carl Eugen Hoyos]

2017-10-21 18:29 GMT+02:00 Nicolas George :
> Le decadi 30 vendémiaire, an CCXXVI, Marton Balint a écrit :
>> I thought filenames in libavformat are limited to 1K anyway
>> because of the AVFormatContext->filename field.

How can I reproduce this?
I tested the data from the ticket from the command line
and it worked fine here.

Or is there maybe no limitation for the data protocol?

>> So if your patch really fixes the ticket, then how? :)

So the original (and the attached) patch do not fix the
ticket for you?
Could you provide some console output to allow me to
better understand the issue?

> Yes, exactly. This limitation was the reason I did not bother handling
> longer lines. I would like to understand how it makes a difference.

New patch attached, please comment.

Carl Eugen
From aa8ff422cfe1d7753f9a90cfbbefe1c02ebf5091 Mon Sep 17 00:00:00 2001
From: Carl Eugen Hoyos 
Date: Sat, 21 Oct 2017 22:02:52 +0200
Subject: [PATCH] lavf/concatdec: Allocate the filenames on the heap.

Allow huge filenames, real-world content can be passed via data: protocol.

Fixes ticket #6761.
---
 libavformat/concatdec.c |   16 +---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/libavformat/concatdec.c b/libavformat/concatdec.c
index 0e18901..b0075ce 100644
--- a/libavformat/concatdec.c
+++ b/libavformat/concatdec.c
@@ -386,16 +386,24 @@ static int concat_read_close(AVFormatContext *avf)
 static int concat_read_header(AVFormatContext *avf)
 {
 ConcatContext *cat = avf->priv_data;
-uint8_t buf[4096];
-uint8_t *cursor, *keyword;
+int buf_size = 50002;
+uint8_t *buf, *cursor, *keyword;
 int ret, line = 0, i;
 unsigned nb_files_alloc = 0;
 ConcatFile *file = NULL;
 int64_t time = 0;
 
+buf = av_malloc(buf_size);
+if (!buf)
+return AVERROR(ENOMEM);
+
 while (1) {
-if ((ret = ff_get_line(avf->pb, buf, sizeof(buf))) <= 0)
+if ((ret = ff_get_line(avf->pb, buf, buf_size)) <= 0)
 break;
+if (ret == buf_size - 1) {
+av_log(avf, AV_LOG_ERROR, "filename too long (>%d)\n", buf_size - 2);
+FAIL(AVERROR_INVALIDDATA);
+}
 line++;
 cursor = buf;
 keyword = get_keyword();
@@ -499,9 +507,11 @@ static int concat_read_header(AVFormatContext *avf)
MATCH_ONE_TO_ONE;
 if ((ret = open_file(avf, 0)) < 0)
 goto fail;
+av_freep();
 return 0;
 
 fail:
+av_freep();
 concat_read_close(avf);
 return ret;
 }
-- 
1.7.10.4

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH]lavf/concatdec: Allocate the filenames on the heap

[Nicolas George]

Le decadi 30 vendémiaire, an CCXXVI, Marton Balint a écrit :
> I thought filenames in libavformat are limited to 1K anyway because of the
> AVFormatContext->filename field. So if your patch really fixes the ticket,
> then how? :)

Yes, exactly. This limitation was the reason I did not bother handling
longer lines. I would like to understand how it makes a difference.

Regards,

-- 
  Nicolas George


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH]lavf/concatdec: Allocate the filenames on the heap

[Marton Balint]

On Fri, 20 Oct 2017, Carl Eugen Hoyos wrote:


Hi!

I believe that the use-case is valid and there is definitely a bug
because no error is shown for too long filenames.
In addition, allocating 50k on the heap seems nicer than 4k on the stack.


50k is not much better than 4k. You should add an ff_get_line variant 
which dynamically allocates the needed buffer.


Also you should free buf somewhere eventually.

I thought filenames in libavformat are limited to 1K anyway because of the 
AVFormatContext->filename field. So if your patch really fixes the ticket, 
then how? :)


Thanks,
Marton
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel