[FFmpeg-devel] lavf/dashenc: Fix file URI handling when deleting files.

Submitted by Andrey Semashev on Nov. 28, 2018, 11:16 a.m.

Details

Message ID 20181128111649.16056-1-andrey.semashev@gmail.com
State New
Headers show

Commit Message

Andrey Semashev Nov. 28, 2018, 11:16 a.m.
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.

It also fixes strerror use, which may not be thread-safe.
---
 libavformat/dashenc.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

Comments

kjeyapal@akamai.com Nov. 28, 2018, 4:47 p.m.
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);

> +        }

>      }

>  }

>
Andrey Semashev Nov. 29, 2018, 11:15 a.m.
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?
Andrey Semashev Nov. 29, 2018, 11:17 a.m.
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.
Andrey Semashev Nov. 29, 2018, 6:19 p.m.
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.
kjeyapal@akamai.com Nov. 30, 2018, 6:08 a.m.
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

Patch hide | download patch | download mbox

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);
+        }
     }
 }