| Message ID | 20181128111649.16056-1-andrey.semashev@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 11/28/18 4:46 PM, Andrey Semashev wrote: > The URI used to open the output streams may be an actual URI with "file" scheme, > according to https://tools.ietf.org/html/rfc8089. This commit makes file > deletion routine recognize file URIs and extract the actual filesystem path > from it. There is already some code in ffmpeg to handle this. It is present in file_delete() function in file.c. We will need to avoid code duplication for the same functionality. One option could be to call avpriv_io_delete() function instead of calling unlink, so that file_delete function gets called. Calling avpriv_io_delete will also make the delete functionality easily extendable for other output protocols. > > It also fixes strerror use, which may not be thread-safe. > --- > libavformat/dashenc.c | 29 +++++++++++++++++++++++++++-- > 1 file changed, 27 insertions(+), 2 deletions(-) > > diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c > index 6ce70e0076..e59fa0944e 100644 > --- a/libavformat/dashenc.c > +++ b/libavformat/dashenc.c > @@ -25,6 +25,7 @@ > #include <unistd.h> > #endif > > +#include "libavutil/error.h" > #include "libavutil/avassert.h" > #include "libavutil/avutil.h" > #include "libavutil/avstring.h" > @@ -1326,8 +1327,32 @@ static void dashenc_delete_file(AVFormatContext *s, char *filename) { > > av_dict_free(&http_opts); > ff_format_io_close(s, &out); > - } else if (unlink(filename) < 0) { > - av_log(s, AV_LOG_ERROR, "failed to delete %s: %s\n", filename, strerror(errno)); > + } else { > + const char* path = filename; > + // Check if the filename is a file URI. https://tools.ietf.org/html/rfc8089#section-2 > + if (av_strncasecmp(path, "file:", sizeof("file:") - 1) == 0) { > + path += sizeof("file:") - 1; > + if (path[0] == '/' && path[1] == '/') { > + // The URI may have an authority part. Check that the authority does not contain > + // a host name. We cannot access filesystem on a different host. > + path += 2; > + if (path[0] != '/') { > + if (strncmp(path, "localhost", sizeof("localhost") - 1) == 0) { > + path += sizeof("localhost") - 1; > + } else { > + av_log(s, AV_LOG_ERROR, "cannot delete file on a remote host: %s\n", filename); > + return; > + } > + } > + } > + } > + > + if (unlink(path) < 0) { > + int err = AVERROR(errno); > + char errbuf[128]; > + av_strerror(err, errbuf, sizeof(errbuf)); > + av_log(s, (err == AVERROR(ENOENT) ? AV_LOG_WARNING : AV_LOG_ERROR), "failed to delete %s: %s\n", path, errbuf); > + } > } > } >
On 11/28/18 7:47 PM, Jeyapal, Karthick wrote: > > On 11/28/18 4:46 PM, Andrey Semashev wrote: >> The URI used to open the output streams may be an actual URI with "file" scheme, >> according to https://tools.ietf.org/html/rfc8089. This commit makes file >> deletion routine recognize file URIs and extract the actual filesystem path >> from it. > There is already some code in ffmpeg to handle this. It is present in file_delete() function in file.c. > We will need to avoid code duplication for the same functionality. One option could be to call avpriv_io_delete() function instead of calling unlink, so that file_delete function gets called. > Calling avpriv_io_delete will also make the delete functionality easily extendable for other output protocols. That would be fine with me, but I'm using Linux. Looking at file_delete (in libavformat/file.c), it looks like it will only work on POSIX systems but not on Windows, since it doesn't have unistd.h. Am I correct? And if so, is avpriv_io_delete still the preferred approach?
On 11/29/18 2:15 PM, Andrey Semashev wrote: > On 11/28/18 7:47 PM, Jeyapal, Karthick wrote: >> >> On 11/28/18 4:46 PM, Andrey Semashev wrote: >>> The URI used to open the output streams may be an actual URI with >>> "file" scheme, >>> according to https://tools.ietf.org/html/rfc8089. This commit makes file >>> deletion routine recognize file URIs and extract the actual >>> filesystem path >>> from it. >> There is already some code in ffmpeg to handle this. It is present in >> file_delete() function in file.c. >> We will need to avoid code duplication for the same functionality. One >> option could be to call avpriv_io_delete() function instead of calling >> unlink, so that file_delete function gets called. >> Calling avpriv_io_delete will also make the delete functionality >> easily extendable for other output protocols. > > That would be fine with me, but I'm using Linux. Looking at file_delete > (in libavformat/file.c), it looks like it will only work on POSIX > systems but not on Windows, since it doesn't have unistd.h. Am I > correct? And if so, is avpriv_io_delete still the preferred approach? Also, that code doesn't seem to support the URI with an authority field and doesn't check the special "localhost" case.
On 11/29/18 2:17 PM, Andrey Semashev wrote: > On 11/29/18 2:15 PM, Andrey Semashev wrote: >> On 11/28/18 7:47 PM, Jeyapal, Karthick wrote: >>> >>> On 11/28/18 4:46 PM, Andrey Semashev wrote: >>>> The URI used to open the output streams may be an actual URI with >>>> "file" scheme, >>>> according to https://tools.ietf.org/html/rfc8089. This commit makes >>>> file >>>> deletion routine recognize file URIs and extract the actual >>>> filesystem path >>>> from it. >>> There is already some code in ffmpeg to handle this. It is present in >>> file_delete() function in file.c. >>> We will need to avoid code duplication for the same functionality. >>> One option could be to call avpriv_io_delete() function instead of >>> calling unlink, so that file_delete function gets called. >>> Calling avpriv_io_delete will also make the delete functionality >>> easily extendable for other output protocols. >> >> That would be fine with me, but I'm using Linux. Looking at >> file_delete (in libavformat/file.c), it looks like it will only work >> on POSIX systems but not on Windows, since it doesn't have unistd.h. >> Am I correct? And if so, is avpriv_io_delete still the preferred >> approach? > > Also, that code doesn't seem to support the URI with an authority field > and doesn't check the special "localhost" case. I've sent a new set of patches that updates both file.c and dashenc.c.
On 11/29/18 11:49 PM, Andrey Semashev wrote: > On 11/29/18 2:17 PM, Andrey Semashev wrote: >> On 11/29/18 2:15 PM, Andrey Semashev wrote: >>> On 11/28/18 7:47 PM, Jeyapal, Karthick wrote: >>>> >>>> On 11/28/18 4:46 PM, Andrey Semashev wrote: >>>>> The URI used to open the output streams may be an actual URI with "file" scheme, >>>>> according to https://tools.ietf.org/html/rfc8089. This commit makes file >>>>> deletion routine recognize file URIs and extract the actual filesystem path >>>>> from it. >>>> There is already some code in ffmpeg to handle this. It is present in file_delete() function in file.c. >>>> We will need to avoid code duplication for the same functionality. One option could be to call avpriv_io_delete() function instead of calling unlink, so that file_delete function gets called. >>>> Calling avpriv_io_delete will also make the delete functionality easily extendable for other output protocols. >>> >>> That would be fine with me, but I'm using Linux. Looking at file_delete (in libavformat/file.c), it looks like it will only work on POSIX systems but not on Windows, since it doesn't have unistd.h. Am I correct? And if so, is avpriv_io_delete still the preferred approach? >> >> Also, that code doesn't seem to support the URI with an authority field and doesn't check the special "localhost" case. > > I've sent a new set of patches that updates both file.c and dashenc.c. Thanks for your understanding. Looks like that will be the clean approach for fixing this problem. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c index 6ce70e0076..e59fa0944e 100644 --- a/libavformat/dashenc.c +++ b/libavformat/dashenc.c @@ -25,6 +25,7 @@ #include <unistd.h> #endif +#include "libavutil/error.h" #include "libavutil/avassert.h" #include "libavutil/avutil.h" #include "libavutil/avstring.h" @@ -1326,8 +1327,32 @@ static void dashenc_delete_file(AVFormatContext *s, char *filename) { av_dict_free(&http_opts); ff_format_io_close(s, &out); - } else if (unlink(filename) < 0) { - av_log(s, AV_LOG_ERROR, "failed to delete %s: %s\n", filename, strerror(errno)); + } else { + const char* path = filename; + // Check if the filename is a file URI. https://tools.ietf.org/html/rfc8089#section-2 + if (av_strncasecmp(path, "file:", sizeof("file:") - 1) == 0) { + path += sizeof("file:") - 1; + if (path[0] == '/' && path[1] == '/') { + // The URI may have an authority part. Check that the authority does not contain + // a host name. We cannot access filesystem on a different host. + path += 2; + if (path[0] != '/') { + if (strncmp(path, "localhost", sizeof("localhost") - 1) == 0) { + path += sizeof("localhost") - 1; + } else { + av_log(s, AV_LOG_ERROR, "cannot delete file on a remote host: %s\n", filename); + return; + } + } + } + } + + if (unlink(path) < 0) { + int err = AVERROR(errno); + char errbuf[128]; + av_strerror(err, errbuf, sizeof(errbuf)); + av_log(s, (err == AVERROR(ENOENT) ? AV_LOG_WARNING : AV_LOG_ERROR), "failed to delete %s: %s\n", path, errbuf); + } } }