* [FFmpeg-devel] [PATCH v1] fftools/ffplay: Resolve input file path before processing
@ 2025-05-13 20:58 Appaji
2025-05-14 11:49 ` Nicolas George
0 siblings, 1 reply; 6+ messages in thread
From: Appaji @ 2025-05-13 20:58 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: Appaji
Fixes ticket: https://trac.ffmpeg.org/ticket/11574
Signed-off-by: Appaji <appaji12368@gmail.com>
---
fftools/ffplay.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/fftools/ffplay.c b/fftools/ffplay.c
index 2a572fc3aa..42f0584b55 100644
--- a/fftools/ffplay.c
+++ b/fftools/ffplay.c
@@ -27,6 +27,7 @@
#include "config_components.h"
#include <math.h>
#include <limits.h>
+#include <stdlib.h>
#include <signal.h>
#include <stdint.h>
@@ -3623,9 +3624,17 @@ static int opt_input_file(void *optctx, const char *filename)
filename, input_filename);
return AVERROR(EINVAL);
}
- if (!strcmp(filename, "-"))
+
+ char resolved_path[PATH_MAX];
+
+ if (!realpath(filename, resolved_path)) {
+ av_log(NULL, AV_LOG_FATAL, "Failed to resolve path for '%s': %s\n", filename, strerror(errno));
+ return AVERROR(errno);
+ }
+
+ if (!strcmp(resolved_path, "-"))
filename = "fd:";
- input_filename = av_strdup(filename);
+ input_filename = av_strdup(resolved_path);
if (!input_filename)
return AVERROR(ENOMEM);
--
2.43.0
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [FFmpeg-devel] [PATCH v1] fftools/ffplay: Resolve input file path before processing
2025-05-13 20:58 [FFmpeg-devel] [PATCH v1] fftools/ffplay: Resolve input file path before processing Appaji
@ 2025-05-14 11:49 ` Nicolas George
2025-05-14 18:06 ` Marton Balint
0 siblings, 1 reply; 6+ messages in thread
From: Nicolas George @ 2025-05-14 11:49 UTC (permalink / raw)
To: FFmpeg development discussions and patches; +Cc: Appaji
Appaji (HE12025-05-14):
> Fixes ticket: https://trac.ffmpeg.org/ticket/11574
>
> Signed-off-by: Appaji <appaji12368@gmail.com>
> ---
> fftools/ffplay.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/fftools/ffplay.c b/fftools/ffplay.c
> index 2a572fc3aa..42f0584b55 100644
> --- a/fftools/ffplay.c
> +++ b/fftools/ffplay.c
> @@ -27,6 +27,7 @@
> #include "config_components.h"
> #include <math.h>
> #include <limits.h>
> +#include <stdlib.h>
> #include <signal.h>
> #include <stdint.h>
>
> @@ -3623,9 +3624,17 @@ static int opt_input_file(void *optctx, const char *filename)
> filename, input_filename);
> return AVERROR(EINVAL);
> }
> - if (!strcmp(filename, "-"))
> +
> + char resolved_path[PATH_MAX];
> +
> + if (!realpath(filename, resolved_path)) {
> + av_log(NULL, AV_LOG_FATAL, "Failed to resolve path for '%s': %s\n", filename, strerror(errno));
> + return AVERROR(errno);
> + }
> +
Hi. Thanks for the patch. Did you test it with non-filenames arguments,
for example http://…?
> + if (!strcmp(resolved_path, "-"))
> filename = "fd:";
This should happen before resolution.
> - input_filename = av_strdup(filename);
> + input_filename = av_strdup(resolved_path);
> if (!input_filename)
> return AVERROR(ENOMEM);
>
On the whole, I think you are going at it wrong: you are only fixing
this for ffplay, not for ffprobe, ffmpeg and other applications built on
the libraries, and resolving the path can have side effects, for example
if you do not have permission on a parent of the current working
directory.
IMO, the correct way would be to add a stat() early in the opening of
the file and test the device number. But that requires changing quite a
lot of things.
Regards,
--
Nicolas George
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [FFmpeg-devel] [PATCH v1] fftools/ffplay: Resolve input file path before processing
2025-05-14 11:49 ` Nicolas George
@ 2025-05-14 18:06 ` Marton Balint
2025-05-15 19:54 ` Appaji Chintimi
0 siblings, 1 reply; 6+ messages in thread
From: Marton Balint @ 2025-05-14 18:06 UTC (permalink / raw)
To: FFmpeg development discussions and patches
On Wed, 14 May 2025, Nicolas George wrote:
> Appaji (HE12025-05-14):
>> Fixes ticket: https://trac.ffmpeg.org/ticket/11574
>>
>> Signed-off-by: Appaji <appaji12368@gmail.com>
>> ---
>> fftools/ffplay.c | 13 +++++++++++--
>> 1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/fftools/ffplay.c b/fftools/ffplay.c
>> index 2a572fc3aa..42f0584b55 100644
>> --- a/fftools/ffplay.c
>> +++ b/fftools/ffplay.c
>> @@ -27,6 +27,7 @@
>> #include "config_components.h"
>> #include <math.h>
>> #include <limits.h>
>> +#include <stdlib.h>
>> #include <signal.h>
>> #include <stdint.h>
>>
>> @@ -3623,9 +3624,17 @@ static int opt_input_file(void *optctx, const char *filename)
>> filename, input_filename);
>> return AVERROR(EINVAL);
>> }
>> - if (!strcmp(filename, "-"))
>> +
>> + char resolved_path[PATH_MAX];
>> +
>> + if (!realpath(filename, resolved_path)) {
>> + av_log(NULL, AV_LOG_FATAL, "Failed to resolve path for '%s': %s\n", filename, strerror(errno));
>> + return AVERROR(errno);
>> + }
>> +
>
> Hi. Thanks for the patch. Did you test it with non-filenames arguments,
> for example http://…?
>
>> + if (!strcmp(resolved_path, "-"))
>> filename = "fd:";
>
> This should happen before resolution.
>
>> - input_filename = av_strdup(filename);
>> + input_filename = av_strdup(resolved_path);
>> if (!input_filename)
>> return AVERROR(ENOMEM);
>>
>
> On the whole, I think you are going at it wrong: you are only fixing
> this for ffplay, not for ffprobe, ffmpeg and other applications built on
> the libraries, and resolving the path can have side effects, for example
> if you do not have permission on a parent of the current working
> directory.
>
> IMO, the correct way would be to add a stat() early in the opening of
> the file and test the device number. But that requires changing quite a
> lot of things.
Agreed. You should improve the probing function to fix the ticket, you can
do a stat in v4l2_read_probe() in libavdevice/v4l2.c, check if it is a
char device and try a V4L2 IOCTL on it to make sure it is a V4L2 device.
Regards,
Marton
>
> Regards,
>
> --
> Nicolas George
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [FFmpeg-devel] [PATCH v1] fftools/ffplay: Resolve input file path before processing
2025-05-14 18:06 ` Marton Balint
@ 2025-05-15 19:54 ` Appaji Chintimi
2025-05-21 5:17 ` Appaji Chintimi
0 siblings, 1 reply; 6+ messages in thread
From: Appaji Chintimi @ 2025-05-15 19:54 UTC (permalink / raw)
To: FFmpeg development discussions and patches
Can you elaborate a bit more why this requires more changes? My
understanding is, to check for different cases and handle them differently:
1. "-" gets replaced with "fd:", and it's passed directly to
"input_filename" without resolving.
2. if the path contains "://", that too gets passed on without resolving it
further (handles http://, https://, ftp:// etc..)
3. Only after these two checks, resolve the "filename" and pass on to
"input_filename".
On Wed, 14 May 2025 at 23:40, Marton Balint <cus@passwd.hu> wrote:
>
>
> On Wed, 14 May 2025, Nicolas George wrote:
>
> > Appaji (HE12025-05-14):
> >> Fixes ticket: https://trac.ffmpeg.org/ticket/11574
> >>
> >> Signed-off-by: Appaji <appaji12368@gmail.com>
> >> ---
> >> fftools/ffplay.c | 13 +++++++++++--
> >> 1 file changed, 11 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/fftools/ffplay.c b/fftools/ffplay.c
> >> index 2a572fc3aa..42f0584b55 100644
> >> --- a/fftools/ffplay.c
> >> +++ b/fftools/ffplay.c
> >> @@ -27,6 +27,7 @@
> >> #include "config_components.h"
> >> #include <math.h>
> >> #include <limits.h>
> >> +#include <stdlib.h>
> >> #include <signal.h>
> >> #include <stdint.h>
> >>
> >> @@ -3623,9 +3624,17 @@ static int opt_input_file(void *optctx, const
> char *filename)
> >> filename, input_filename);
> >> return AVERROR(EINVAL);
> >> }
> >> - if (!strcmp(filename, "-"))
> >> +
> >> + char resolved_path[PATH_MAX];
> >> +
> >> + if (!realpath(filename, resolved_path)) {
> >> + av_log(NULL, AV_LOG_FATAL, "Failed to resolve path for '%s':
> %s\n", filename, strerror(errno));
> >> + return AVERROR(errno);
> >> + }
> >> +
> >
> > Hi. Thanks for the patch. Did you test it with non-filenames arguments,
> > for example http://…?
> >
> >> + if (!strcmp(resolved_path, "-"))
> >> filename = "fd:";
> >
> > This should happen before resolution.
> >
> >> - input_filename = av_strdup(filename);
> >> + input_filename = av_strdup(resolved_path);
> >> if (!input_filename)
> >> return AVERROR(ENOMEM);
> >>
> >
> > On the whole, I think you are going at it wrong: you are only fixing
> > this for ffplay, not for ffprobe, ffmpeg and other applications built on
> > the libraries, and resolving the path can have side effects, for example
> > if you do not have permission on a parent of the current working
> > directory.
> >
> > IMO, the correct way would be to add a stat() early in the opening of
> > the file and test the device number. But that requires changing quite a
> > lot of things.
>
> Agreed. You should improve the probing function to fix the ticket, you can
> do a stat in v4l2_read_probe() in libavdevice/v4l2.c, check if it is a
> char device and try a V4L2 IOCTL on it to make sure it is a V4L2 device.
>
> Regards,
> Marton
>
> >
> > Regards,
> >
> > --
> > Nicolas George
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [FFmpeg-devel] [PATCH v1] fftools/ffplay: Resolve input file path before processing
2025-05-15 19:54 ` Appaji Chintimi
@ 2025-05-21 5:17 ` Appaji Chintimi
2025-05-21 6:50 ` Nicolas George
0 siblings, 1 reply; 6+ messages in thread
From: Appaji Chintimi @ 2025-05-21 5:17 UTC (permalink / raw)
To: FFmpeg development discussions and patches
Hello, Can I get some eyes on this please?
On Fri, 16 May 2025 at 01:24, Appaji Chintimi <appaji12368@gmail.com> wrote:
>
> Can you elaborate a bit more why this requires more changes? My understanding is, to check for different cases and handle them differently:
>
> 1. "-" gets replaced with "fd:", and it's passed directly to "input_filename" without resolving.
> 2. if the path contains "://", that too gets passed on without resolving it further (handles http://, https://, ftp:// etc..)
> 3. Only after these two checks, resolve the "filename" and pass on to "input_filename".
>
> On Wed, 14 May 2025 at 23:40, Marton Balint <cus@passwd.hu> wrote:
>>
>>
>>
>> On Wed, 14 May 2025, Nicolas George wrote:
>>
>> > Appaji (HE12025-05-14):
>> >> Fixes ticket: https://trac.ffmpeg.org/ticket/11574
>> >>
>> >> Signed-off-by: Appaji <appaji12368@gmail.com>
>> >> ---
>> >> fftools/ffplay.c | 13 +++++++++++--
>> >> 1 file changed, 11 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/fftools/ffplay.c b/fftools/ffplay.c
>> >> index 2a572fc3aa..42f0584b55 100644
>> >> --- a/fftools/ffplay.c
>> >> +++ b/fftools/ffplay.c
>> >> @@ -27,6 +27,7 @@
>> >> #include "config_components.h"
>> >> #include <math.h>
>> >> #include <limits.h>
>> >> +#include <stdlib.h>
>> >> #include <signal.h>
>> >> #include <stdint.h>
>> >>
>> >> @@ -3623,9 +3624,17 @@ static int opt_input_file(void *optctx, const char *filename)
>> >> filename, input_filename);
>> >> return AVERROR(EINVAL);
>> >> }
>> >> - if (!strcmp(filename, "-"))
>> >> +
>> >> + char resolved_path[PATH_MAX];
>> >> +
>> >> + if (!realpath(filename, resolved_path)) {
>> >> + av_log(NULL, AV_LOG_FATAL, "Failed to resolve path for '%s': %s\n", filename, strerror(errno));
>> >> + return AVERROR(errno);
>> >> + }
>> >> +
>> >
>> > Hi. Thanks for the patch. Did you test it with non-filenames arguments,
>> > for example http://…?
>> >
>> >> + if (!strcmp(resolved_path, "-"))
>> >> filename = "fd:";
>> >
>> > This should happen before resolution.
>> >
>> >> - input_filename = av_strdup(filename);
>> >> + input_filename = av_strdup(resolved_path);
>> >> if (!input_filename)
>> >> return AVERROR(ENOMEM);
>> >>
>> >
>> > On the whole, I think you are going at it wrong: you are only fixing
>> > this for ffplay, not for ffprobe, ffmpeg and other applications built on
>> > the libraries, and resolving the path can have side effects, for example
>> > if you do not have permission on a parent of the current working
>> > directory.
>> >
>> > IMO, the correct way would be to add a stat() early in the opening of
>> > the file and test the device number. But that requires changing quite a
>> > lot of things.
>>
>> Agreed. You should improve the probing function to fix the ticket, you can
>> do a stat in v4l2_read_probe() in libavdevice/v4l2.c, check if it is a
>> char device and try a V4L2 IOCTL on it to make sure it is a V4L2 device.
>>
>> Regards,
>> Marton
>>
>> >
>> > Regards,
>> >
>> > --
>> > Nicolas George
>> > _______________________________________________
>> > ffmpeg-devel mailing list
>> > ffmpeg-devel@ffmpeg.org
>> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> >
>> > To unsubscribe, visit link above, or email
>> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [FFmpeg-devel] [PATCH v1] fftools/ffplay: Resolve input file path before processing
2025-05-21 5:17 ` Appaji Chintimi
@ 2025-05-21 6:50 ` Nicolas George
0 siblings, 0 replies; 6+ messages in thread
From: Nicolas George @ 2025-05-21 6:50 UTC (permalink / raw)
To: FFmpeg development discussions and patches
Appaji Chintimi (HE12025-05-21):
> Hello, Can I get some eyes on this please?
You just had two set of eyes on this already.
If you think we made mistakes in our review, prove it: post the full
console output of (1) reading a network stream, (2) reading from a pipe,
(3) reading from a directory where one of the parents is not
traversable both with and without your patch.
> On Fri, 16 May 2025 at 01:24, Appaji Chintimi <appaji12368@gmail.com> wrote:
And stop top-posting. If you do not know what it means look it up.
--
Nicolas George
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-05-21 6:51 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-05-13 20:58 [FFmpeg-devel] [PATCH v1] fftools/ffplay: Resolve input file path before processing Appaji
2025-05-14 11:49 ` Nicolas George
2025-05-14 18:06 ` Marton Balint
2025-05-15 19:54 ` Appaji Chintimi
2025-05-21 5:17 ` Appaji Chintimi
2025-05-21 6:50 ` Nicolas George
Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
This inbox may be cloned and mirrored by anyone:
git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git
# If you have public-inbox 1.1+ installed, you may
# initialize and index your mirror using the following commands:
public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
ffmpegdev@gitmailbox.com
public-inbox-index ffmpegdev
Example config snippet for mirrors.
AGPL code for this site: git clone https://public-inbox.org/public-inbox.git