| Message ID | 1568595818-7943-1-git-send-email-lance.lmwang@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
> 在 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".
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
> 在 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".
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
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".
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
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
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 --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);