* [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