diff mbox

[FFmpeg-devel,v1,1/4] avutil/avstring: support input path is a null pointer or empty string

Message ID 1568595818-7943-1-git-send-email-lance.lmwang@gmail.com
State Superseded
Headers show

Commit Message

Lance Wang Sept. 16, 2019, 1:03 a.m. UTC
From: Limin Wang <lance.lmwang@gmail.com>

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavutil/avstring.c | 12 ++++++++----
 libavutil/avstring.h | 13 +++++++++----
 2 files changed, 17 insertions(+), 8 deletions(-)

Comments

Steven Liu Sept. 16, 2019, 1:46 a.m. UTC | #1
> 在 2019年9月16日,上午9:03,lance.lmwang@gmail.com 写道:
> 
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
> libavutil/avstring.c | 12 ++++++++----
> libavutil/avstring.h | 13 +++++++++----
> 2 files changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/libavutil/avstring.c b/libavutil/avstring.c
> index 4c068f5bc5..9fddd0c77b 100644
> --- a/libavutil/avstring.c
> +++ b/libavutil/avstring.c
> @@ -257,8 +257,12 @@ char *av_strireplace(const char *str, const char *from, const char *to)
> 
> const char *av_basename(const char *path)
> {
> -    char *p = strrchr(path, '/');
> +    char *p = NULL;
> +
> +    if (!path || *path == '\0')
> +        return ".";
> 
> +    p = strrchr(path, '/');
> #if HAVE_DOS_PATHS
>     char *q = strrchr(path, '\\');
>     char *d = strchr(path, ':');
> @@ -274,11 +278,11 @@ const char *av_basename(const char *path)
> 
> const char *av_dirname(char *path)
> {
> -    char *p = strrchr(path, '/');
> +    char *p = path != NULL ? strrchr(path, '/') : NULL;
> 
> #if HAVE_DOS_PATHS
> -    char *q = strrchr(path, '\\');
> -    char *d = strchr(path, ':');
> +    char *q = path != NULL ? strrchr(path, '\\') : NULL;
> +    char *d = path != NULL ? strchr(path, ':')  : NULL;;
remove one ‘;'
> 
>     d = d ? d + 1 : d;
> 
> diff --git a/libavutil/avstring.h b/libavutil/avstring.h
> index 37dd4e2da0..274335cfb9 100644
> --- a/libavutil/avstring.h
> +++ b/libavutil/avstring.h
> @@ -274,16 +274,21 @@ char *av_strireplace(const char *str, const char *from, const char *to);
> 
> /**
>  * Thread safe basename.
> - * @param path the path, on DOS both \ and / are considered separators.
> + * @param path the string to parse, on DOS both \ and / are considered separators.
>  * @return pointer to the basename substring.
> + * If path does not contain a slash, the function returns a copy of path.
> + * If path is a NULL pointer or points to an empty string, a pointer
> + * to a string "." is returned.
>  */
> const char *av_basename(const char *path);
> 
> /**
>  * Thread safe dirname.
> - * @param path the path, on DOS both \ and / are considered separators.
> - * @return the path with the separator replaced by the string terminator or ".".
> - * @note the function may change the input string.
> + * @param path the string to parse, on DOS both \ and / are considered separators.
> + * @return A pointer to a string that's the parent directory of path.
> + * If path is a NULL pointer or points to an empty string, a pointer
> + * to a string "." is returned.
> + * @note the function may modify the contents of the path, so copies should be passed.
>  */
> const char *av_dirname(char *path);
> 
> -- 
> 2.21.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".
Tomas Härdin Sept. 16, 2019, 8:19 a.m. UTC | #2
mån 2019-09-16 klockan 09:03 +0800 skrev lance.lmwang@gmail.com:
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  libavutil/avstring.c | 12 ++++++++----
>  libavutil/avstring.h | 13 +++++++++----
>  2 files changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/libavutil/avstring.c b/libavutil/avstring.c
> index 4c068f5bc5..9fddd0c77b 100644
> --- a/libavutil/avstring.c
> +++ b/libavutil/avstring.c
> @@ -257,8 +257,12 @@ char *av_strireplace(const char *str, const char
> *from, const char *to)
>  
>  const char *av_basename(const char *path)
>  {
> -    char *p = strrchr(path, '/');
> +    char *p = NULL;
> +
> +    if (!path || *path == '\0')
> +        return ".";

I will note here that this kind of thing would go great with a contract
on the function prototype, so that callers could formally verify that
they can indeed remove the NULL checks, and that the result of
av_basename() is always a valid string..

The patch itself is probably fine

/Tomas
Steven Liu Sept. 16, 2019, 8:43 a.m. UTC | #3
> 在 2019年9月16日,下午4:19,Tomas Härdin <tjoppen@acc.umu.se> 写道:
> 
> mån 2019-09-16 klockan 09:03 +0800 skrev lance.lmwang@gmail.com:
>> From: Limin Wang <lance.lmwang@gmail.com>
>> 
>> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
>> ---
>> libavutil/avstring.c | 12 ++++++++----
>> libavutil/avstring.h | 13 +++++++++----
>> 2 files changed, 17 insertions(+), 8 deletions(-)
>> 
>> diff --git a/libavutil/avstring.c b/libavutil/avstring.c
>> index 4c068f5bc5..9fddd0c77b 100644
>> --- a/libavutil/avstring.c
>> +++ b/libavutil/avstring.c
>> @@ -257,8 +257,12 @@ char *av_strireplace(const char *str, const char
>> *from, const char *to)
>> 
>> const char *av_basename(const char *path)
>> {
>> -    char *p = strrchr(path, '/');
>> +    char *p = NULL;
>> +
>> +    if (!path || *path == '\0')
>> +        return ".";
> 
> I will note here that this kind of thing would go great with a contract
> on the function prototype, so that callers could formally verify that
> they can indeed remove the NULL checks, and that the result of
> av_basename() is always a valid string..
> 
> The patch itself is probably fine
+1
> 
> /Tomas
> 
> _______________________________________________
> 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".
Marton Balint Sept. 17, 2019, 4:22 p.m. UTC | #4
On Mon, 16 Sep 2019, Tomas Härdin wrote:

> mån 2019-09-16 klockan 09:03 +0800 skrev lance.lmwang@gmail.com:
>> From: Limin Wang <lance.lmwang@gmail.com>
>> 
>> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
>> ---
>>  libavutil/avstring.c | 12 ++++++++----
>>  libavutil/avstring.h | 13 +++++++++----
>>  2 files changed, 17 insertions(+), 8 deletions(-)
>> 
>> diff --git a/libavutil/avstring.c b/libavutil/avstring.c
>> index 4c068f5bc5..9fddd0c77b 100644
>> --- a/libavutil/avstring.c
>> +++ b/libavutil/avstring.c
>> @@ -257,8 +257,12 @@ char *av_strireplace(const char *str, const char
>> *from, const char *to)
>>
>>  const char *av_basename(const char *path)
>>  {
>> -    char *p = strrchr(path, '/');
>> +    char *p = NULL;
>> +
>> +    if (!path || *path == '\0')
>> +        return ".";
>
> I will note here that this kind of thing would go great with a contract
> on the function prototype, so that callers could formally verify that
> they can indeed remove the NULL checks, and that the result of
> av_basename() is always a valid string..

This is basename, not dirname. We should not return an 
arbitrary (valid) value for invalid inputs.

> The patch itself is probably fine

I disagree here.

Regards,
Marton
Lance Wang Sept. 17, 2019, 10:41 p.m. UTC | #5
On Tue, Sep 17, 2019 at 06:22:39PM +0200, Marton Balint wrote:
> 
> 
> On Mon, 16 Sep 2019, Tomas Härdin wrote:
> 
> >mån 2019-09-16 klockan 09:03 +0800 skrev lance.lmwang@gmail.com:
> >>From: Limin Wang <lance.lmwang@gmail.com>
> >>
> >>Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> >>---
> >> libavutil/avstring.c | 12 ++++++++----
> >> libavutil/avstring.h | 13 +++++++++----
> >> 2 files changed, 17 insertions(+), 8 deletions(-)
> >>
> >>diff --git a/libavutil/avstring.c b/libavutil/avstring.c
> >>index 4c068f5bc5..9fddd0c77b 100644
> >>--- a/libavutil/avstring.c
> >>+++ b/libavutil/avstring.c
> >>@@ -257,8 +257,12 @@ char *av_strireplace(const char *str, const char
> >>*from, const char *to)
> >>
> >> const char *av_basename(const char *path)
> >> {
> >>-    char *p = strrchr(path, '/');
> >>+    char *p = NULL;
> >>+
> >>+    if (!path || *path == '\0')
> >>+        return ".";
> >
> >I will note here that this kind of thing would go great with a contract
> >on the function prototype, so that callers could formally verify that
> >they can indeed remove the NULL checks, and that the result of
> >av_basename() is always a valid string..
> 
> This is basename, not dirname. We should not return an arbitrary
> (valid) value for invalid inputs.

basename and dirname is supported by Linux and OSX system by <libgen.h>,
I consider to make the interface is consistent with the standard api first,
then it's time to change to invoke the system api if it's support. I
have read the implementaion in linux, it's more robust and tested. for
example, the current code haven't process multiple `/' characters.

You can get more descrioption about the system api by below command:
man 3 basename 

> 
> >The patch itself is probably fine
> 
> I disagree here.
> 
> Regards,
> Marton
> _______________________________________________
> 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".
Marton Balint Sept. 18, 2019, 6:25 p.m. UTC | #6
On Wed, 18 Sep 2019, Limin Wang wrote:

> On Tue, Sep 17, 2019 at 06:22:39PM +0200, Marton Balint wrote:
>> 
>> 
>> On Mon, 16 Sep 2019, Tomas Härdin wrote:
>> 
>> >mån 2019-09-16 klockan 09:03 +0800 skrev lance.lmwang@gmail.com:
>> >>From: Limin Wang <lance.lmwang@gmail.com>
>> >>
>> >>Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
>> >>---
>> >> libavutil/avstring.c | 12 ++++++++----
>> >> libavutil/avstring.h | 13 +++++++++----
>> >> 2 files changed, 17 insertions(+), 8 deletions(-)
>> >>
>> >>diff --git a/libavutil/avstring.c b/libavutil/avstring.c
>> >>index 4c068f5bc5..9fddd0c77b 100644
>> >>--- a/libavutil/avstring.c
>> >>+++ b/libavutil/avstring.c
>> >>@@ -257,8 +257,12 @@ char *av_strireplace(const char *str, const char
>> >>*from, const char *to)
>> >>
>> >> const char *av_basename(const char *path)
>> >> {
>> >>-    char *p = strrchr(path, '/');
>> >>+    char *p = NULL;
>> >>+
>> >>+    if (!path || *path == '\0')
>> >>+        return ".";
>> >
>> >I will note here that this kind of thing would go great with a contract
>> >on the function prototype, so that callers could formally verify that
>> >they can indeed remove the NULL checks, and that the result of
>> >av_basename() is always a valid string..
>> 
>> This is basename, not dirname. We should not return an arbitrary
>> (valid) value for invalid inputs.
>
> basename and dirname is supported by Linux and OSX system by <libgen.h>,
> I consider to make the interface is consistent with the standard api first,
> then it's time to change to invoke the system api if it's support. I
> have read the implementaion in linux, it's more robust and tested. for
> example, the current code haven't process multiple `/' characters.
>
> You can get more descrioption about the system api by below command:
> man 3 basename

OK thanks for explaining, so "." is not completely arbitrary, it mimics 
the standard C API. Please add this as the reason of your change into the 
commit message. I am still not sure if it is good practice though, but I 
am fine with it if nobody else sees this problematic.

Regards,
Marton
Tomas Härdin Sept. 18, 2019, 6:34 p.m. UTC | #7
ons 2019-09-18 klockan 06:41 +0800 skrev Limin Wang:
> On Tue, Sep 17, 2019 at 06:22:39PM +0200, Marton Balint wrote:
> > 
> > On Mon, 16 Sep 2019, Tomas Härdin wrote:
> > 
> > > mån 2019-09-16 klockan 09:03 +0800 skrev lance.lmwang@gmail.com:
> > > > From: Limin Wang <lance.lmwang@gmail.com>
> > > > 
> > > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > > > ---
> > > > libavutil/avstring.c | 12 ++++++++----
> > > > libavutil/avstring.h | 13 +++++++++----
> > > > 2 files changed, 17 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/libavutil/avstring.c b/libavutil/avstring.c
> > > > index 4c068f5bc5..9fddd0c77b 100644
> > > > --- a/libavutil/avstring.c
> > > > +++ b/libavutil/avstring.c
> > > > @@ -257,8 +257,12 @@ char *av_strireplace(const char *str, const char
> > > > *from, const char *to)
> > > > 
> > > > const char *av_basename(const char *path)
> > > > {
> > > > -    char *p = strrchr(path, '/');
> > > > +    char *p = NULL;
> > > > +
> > > > +    if (!path || *path == '\0')
> > > > +        return ".";
> > > 
> > > I will note here that this kind of thing would go great with a contract
> > > on the function prototype, so that callers could formally verify that
> > > they can indeed remove the NULL checks, and that the result of
> > > av_basename() is always a valid string..
> > 
> > This is basename, not dirname. We should not return an arbitrary
> > (valid) value for invalid inputs.

Sure. Better would of course be to not be able to parse invalid input
at all

> basename and dirname is supported by Linux and OSX system by <libgen.h>,
> I consider to make the interface is consistent with the standard api first,
> then it's time to change to invoke the system api if it's support. I
> have read the implementaion in linux, it's more robust and tested. for
> example, the current code haven't process multiple `/' characters.

*cries in langsec*

/Tomas
Lance Wang Sept. 19, 2019, 1:08 a.m. UTC | #8
On Wed, Sep 18, 2019 at 08:25:40PM +0200, Marton Balint wrote:
> 
> 
> On Wed, 18 Sep 2019, Limin Wang wrote:
> 
> >On Tue, Sep 17, 2019 at 06:22:39PM +0200, Marton Balint wrote:
> >>
> >>
> >>On Mon, 16 Sep 2019, Tomas Härdin wrote:
> >>
> >>>mån 2019-09-16 klockan 09:03 +0800 skrev lance.lmwang@gmail.com:
> >>>>From: Limin Wang <lance.lmwang@gmail.com>
> >>>>
> >>>>Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> >>>>---
> >>>> libavutil/avstring.c | 12 ++++++++----
> >>>> libavutil/avstring.h | 13 +++++++++----
> >>>> 2 files changed, 17 insertions(+), 8 deletions(-)
> >>>>
> >>>>diff --git a/libavutil/avstring.c b/libavutil/avstring.c
> >>>>index 4c068f5bc5..9fddd0c77b 100644
> >>>>--- a/libavutil/avstring.c
> >>>>+++ b/libavutil/avstring.c
> >>>>@@ -257,8 +257,12 @@ char *av_strireplace(const char *str, const char
> >>>>*from, const char *to)
> >>>>
> >>>> const char *av_basename(const char *path)
> >>>> {
> >>>>-    char *p = strrchr(path, '/');
> >>>>+    char *p = NULL;
> >>>>+
> >>>>+    if (!path || *path == '\0')
> >>>>+        return ".";
> >>>
> >>>I will note here that this kind of thing would go great with a contract
> >>>on the function prototype, so that callers could formally verify that
> >>>they can indeed remove the NULL checks, and that the result of
> >>>av_basename() is always a valid string..
> >>
> >>This is basename, not dirname. We should not return an arbitrary
> >>(valid) value for invalid inputs.
> >
> >basename and dirname is supported by Linux and OSX system by <libgen.h>,
> >I consider to make the interface is consistent with the standard api first,
> >then it's time to change to invoke the system api if it's support. I
> >have read the implementaion in linux, it's more robust and tested. for
> >example, the current code haven't process multiple `/' characters.
> >
> >You can get more descrioption about the system api by below command:
> >man 3 basename
> 
> OK thanks for explaining, so "." is not completely arbitrary, it
> mimics the standard C API. Please add this as the reason of your
> change into the commit message. I am still not sure if it is good
> practice though, but I am fine with it if nobody else sees this
> problematic.

Thanks for your comments anyway, I'll update the commit message for the patch.

> 
> Regards,
> Marton
> _______________________________________________
> 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".
diff mbox

Patch

diff --git a/libavutil/avstring.c b/libavutil/avstring.c
index 4c068f5bc5..9fddd0c77b 100644
--- a/libavutil/avstring.c
+++ b/libavutil/avstring.c
@@ -257,8 +257,12 @@  char *av_strireplace(const char *str, const char *from, const char *to)
 
 const char *av_basename(const char *path)
 {
-    char *p = strrchr(path, '/');
+    char *p = NULL;
+
+    if (!path || *path == '\0')
+        return ".";
 
+    p = strrchr(path, '/');
 #if HAVE_DOS_PATHS
     char *q = strrchr(path, '\\');
     char *d = strchr(path, ':');
@@ -274,11 +278,11 @@  const char *av_basename(const char *path)
 
 const char *av_dirname(char *path)
 {
-    char *p = strrchr(path, '/');
+    char *p = path != NULL ? strrchr(path, '/') : NULL;
 
 #if HAVE_DOS_PATHS
-    char *q = strrchr(path, '\\');
-    char *d = strchr(path, ':');
+    char *q = path != NULL ? strrchr(path, '\\') : NULL;
+    char *d = path != NULL ? strchr(path, ':')  : NULL;;
 
     d = d ? d + 1 : d;
 
diff --git a/libavutil/avstring.h b/libavutil/avstring.h
index 37dd4e2da0..274335cfb9 100644
--- a/libavutil/avstring.h
+++ b/libavutil/avstring.h
@@ -274,16 +274,21 @@  char *av_strireplace(const char *str, const char *from, const char *to);
 
 /**
  * Thread safe basename.
- * @param path the path, on DOS both \ and / are considered separators.
+ * @param path the string to parse, on DOS both \ and / are considered separators.
  * @return pointer to the basename substring.
+ * If path does not contain a slash, the function returns a copy of path.
+ * If path is a NULL pointer or points to an empty string, a pointer
+ * to a string "." is returned.
  */
 const char *av_basename(const char *path);
 
 /**
  * Thread safe dirname.
- * @param path the path, on DOS both \ and / are considered separators.
- * @return the path with the separator replaced by the string terminator or ".".
- * @note the function may change the input string.
+ * @param path the string to parse, on DOS both \ and / are considered separators.
+ * @return A pointer to a string that's the parent directory of path.
+ * If path is a NULL pointer or points to an empty string, a pointer
+ * to a string "." is returned.
+ * @note the function may modify the contents of the path, so copies should be passed.
  */
 const char *av_dirname(char *path);