| Message ID | 20170707021850.128743-1-wtc@google.com |
|---|---|
| State | Accepted |
| Commit | 2f84f40d451cec38571ef4a0c5d83d642800ce12 |
| Headers | show |
On Fri, Jul 7, 2017 at 9:18 AM, Wan-Teh Chang <wtc-at-google.com@ffmpeg.org> wrote: > In url_find_protocol(), proto_str is either "file" or a string > consisting of only the characters in URL_SCHEME_CHARS, which does not > include ','. Therefore the strchr(proto_str, ',') call always returns > NULL. > > Note: The code was added in commit > 6161c41817f6e53abb3021d67ca0f19def682718. > > Signed-off-by: Wan-Teh Chang <wtc@google.com> > --- > libavformat/avio.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/libavformat/avio.c b/libavformat/avio.c > index 1e79c9dd5c..64248e098b 100644 > --- a/libavformat/avio.c > +++ b/libavformat/avio.c > @@ -263,8 +263,6 @@ static const struct URLProtocol *url_find_protocol(const char *filename) > av_strlcpy(proto_str, filename, > FFMIN(proto_len + 1, sizeof(proto_str))); > > - if ((ptr = strchr(proto_str, ','))) > - *ptr = '\0'; What about handling "subfile," ?
Hi Muhammad, On Thu, Jul 6, 2017 at 7:56 PM, Muhammad Faiz <mfcc64@gmail.com> wrote: > On Fri, Jul 7, 2017 at 9:18 AM, Wan-Teh Chang > <wtc-at-google.com@ffmpeg.org> wrote: >> In url_find_protocol(), proto_str is either "file" or a string >> consisting of only the characters in URL_SCHEME_CHARS, which does not >> include ','. Therefore the strchr(proto_str, ',') call always returns >> NULL. >> >> Note: The code was added in commit >> 6161c41817f6e53abb3021d67ca0f19def682718. >> >> Signed-off-by: Wan-Teh Chang <wtc@google.com> >> --- >> libavformat/avio.c | 2 -- >> 1 file changed, 2 deletions(-) >> >> diff --git a/libavformat/avio.c b/libavformat/avio.c >> index 1e79c9dd5c..64248e098b 100644 >> --- a/libavformat/avio.c >> +++ b/libavformat/avio.c >> @@ -263,8 +263,6 @@ static const struct URLProtocol *url_find_protocol(const char *filename) >> av_strlcpy(proto_str, filename, >> FFMIN(proto_len + 1, sizeof(proto_str))); >> >> - if ((ptr = strchr(proto_str, ','))) >> - *ptr = '\0'; > > What about handling "subfile," ? I don't know what url_find_protocol() is intended to do, but I can show you what it actually does. Here is the relevant code in libavformat/avio.c: ====================================================================== #define URL_SCHEME_CHARS \ "abcdefghijklmnopqrstuvwxyz" \ "ABCDEFGHIJKLMNOPQRSTUVWXYZ" \ "0123456789+-." static const struct URLProtocol *url_find_protocol(const char *filename) { const URLProtocol **protocols; char proto_str[128], proto_nested[128], *ptr; size_t proto_len = strspn(filename, URL_SCHEME_CHARS); int i; if (filename[proto_len] != ':' && (strncmp(filename, "subfile,", 8) || !strchr(filename + proto_len + 1, ':')) || is_dos_path(filename)) strcpy(proto_str, "file"); else av_strlcpy(proto_str, filename, FFMIN(proto_len + 1, sizeof(proto_str))); if ((ptr = strchr(proto_str, ','))) *ptr = '\0'; ====================================================================== Since I don't know how "subfile," should be handled by url_find_protocol(), I ran the following three test inputs in the debugger: If |filename| is "subfile,", then proto_len is 7 and the if branch copies "file" into proto_str. If |filename| is "subfile,abcdefg", then proto_len is 7 and the if branch copies "file" into proto_str. If |filename| is "subfile,abcdefg:hijk", then proto_len is 7 and the else branch copies "subfile" into proto_str. Is this the expected behavior? Note: The code snippet shows proto_str cannot possibly contain ','. This is why the strchr(proto_str, ',') call always returns NULL. Thanks, Wan-Teh Chang
On Fri, Jul 7, 2017 at 11:27 AM, Wan-Teh Chang <wtc-at-google.com@ffmpeg.org> wrote: > Hi Muhammad, > > On Thu, Jul 6, 2017 at 7:56 PM, Muhammad Faiz <mfcc64@gmail.com> wrote: >> On Fri, Jul 7, 2017 at 9:18 AM, Wan-Teh Chang >> <wtc-at-google.com@ffmpeg.org> wrote: >>> In url_find_protocol(), proto_str is either "file" or a string >>> consisting of only the characters in URL_SCHEME_CHARS, which does not >>> include ','. Therefore the strchr(proto_str, ',') call always returns >>> NULL. >>> >>> Note: The code was added in commit >>> 6161c41817f6e53abb3021d67ca0f19def682718. >>> >>> Signed-off-by: Wan-Teh Chang <wtc@google.com> >>> --- >>> libavformat/avio.c | 2 -- >>> 1 file changed, 2 deletions(-) >>> >>> diff --git a/libavformat/avio.c b/libavformat/avio.c >>> index 1e79c9dd5c..64248e098b 100644 >>> --- a/libavformat/avio.c >>> +++ b/libavformat/avio.c >>> @@ -263,8 +263,6 @@ static const struct URLProtocol *url_find_protocol(const char *filename) >>> av_strlcpy(proto_str, filename, >>> FFMIN(proto_len + 1, sizeof(proto_str))); >>> >>> - if ((ptr = strchr(proto_str, ','))) >>> - *ptr = '\0'; >> >> What about handling "subfile," ? > > I don't know what url_find_protocol() is intended to do, but I can > show you what it actually does. > > Here is the relevant code in libavformat/avio.c: > > ====================================================================== > #define URL_SCHEME_CHARS \ > "abcdefghijklmnopqrstuvwxyz" \ > "ABCDEFGHIJKLMNOPQRSTUVWXYZ" \ > "0123456789+-." > > static const struct URLProtocol *url_find_protocol(const char *filename) > { > const URLProtocol **protocols; > char proto_str[128], proto_nested[128], *ptr; > size_t proto_len = strspn(filename, URL_SCHEME_CHARS); > int i; > > if (filename[proto_len] != ':' && > (strncmp(filename, "subfile,", 8) || !strchr(filename + > proto_len + 1, ':')) || > is_dos_path(filename)) > strcpy(proto_str, "file"); > else > av_strlcpy(proto_str, filename, > FFMIN(proto_len + 1, sizeof(proto_str))); > > if ((ptr = strchr(proto_str, ','))) > *ptr = '\0'; > ====================================================================== > > Since I don't know how "subfile," should be handled by > url_find_protocol(), I ran the following three test inputs in the > debugger: > > If |filename| is "subfile,", then proto_len is 7 and the if branch > copies "file" into proto_str. > > If |filename| is "subfile,abcdefg", then proto_len is 7 and the if > branch copies "file" into proto_str. > > If |filename| is "subfile,abcdefg:hijk", then proto_len is 7 and the > else branch copies "subfile" into proto_str. Oh, I see. I was wrong. > > Is this the expected behavior? I don't know. However it is the previous behavior, so LGTM. Thank's.
On Fri, Jul 7, 2017 at 1:32 PM, Muhammad Faiz <mfcc64@gmail.com> wrote: > On Fri, Jul 7, 2017 at 11:27 AM, Wan-Teh Chang > <wtc-at-google.com@ffmpeg.org> wrote: >> Hi Muhammad, >> >> On Thu, Jul 6, 2017 at 7:56 PM, Muhammad Faiz <mfcc64@gmail.com> wrote: >>> On Fri, Jul 7, 2017 at 9:18 AM, Wan-Teh Chang >>> <wtc-at-google.com@ffmpeg.org> wrote: >>>> In url_find_protocol(), proto_str is either "file" or a string >>>> consisting of only the characters in URL_SCHEME_CHARS, which does not >>>> include ','. Therefore the strchr(proto_str, ',') call always returns >>>> NULL. >>>> >>>> Note: The code was added in commit >>>> 6161c41817f6e53abb3021d67ca0f19def682718. >>>> >>>> Signed-off-by: Wan-Teh Chang <wtc@google.com> >>>> --- >>>> libavformat/avio.c | 2 -- >>>> 1 file changed, 2 deletions(-) >>>> >>>> diff --git a/libavformat/avio.c b/libavformat/avio.c >>>> index 1e79c9dd5c..64248e098b 100644 >>>> --- a/libavformat/avio.c >>>> +++ b/libavformat/avio.c >>>> @@ -263,8 +263,6 @@ static const struct URLProtocol *url_find_protocol(const char *filename) >>>> av_strlcpy(proto_str, filename, >>>> FFMIN(proto_len + 1, sizeof(proto_str))); >>>> >>>> - if ((ptr = strchr(proto_str, ','))) >>>> - *ptr = '\0'; >>> >>> What about handling "subfile," ? >> >> I don't know what url_find_protocol() is intended to do, but I can >> show you what it actually does. >> >> Here is the relevant code in libavformat/avio.c: >> >> ====================================================================== >> #define URL_SCHEME_CHARS \ >> "abcdefghijklmnopqrstuvwxyz" \ >> "ABCDEFGHIJKLMNOPQRSTUVWXYZ" \ >> "0123456789+-." >> >> static const struct URLProtocol *url_find_protocol(const char *filename) >> { >> const URLProtocol **protocols; >> char proto_str[128], proto_nested[128], *ptr; >> size_t proto_len = strspn(filename, URL_SCHEME_CHARS); >> int i; >> >> if (filename[proto_len] != ':' && >> (strncmp(filename, "subfile,", 8) || !strchr(filename + >> proto_len + 1, ':')) || >> is_dos_path(filename)) >> strcpy(proto_str, "file"); >> else >> av_strlcpy(proto_str, filename, >> FFMIN(proto_len + 1, sizeof(proto_str))); >> >> if ((ptr = strchr(proto_str, ','))) >> *ptr = '\0'; >> ====================================================================== >> >> Since I don't know how "subfile," should be handled by >> url_find_protocol(), I ran the following three test inputs in the >> debugger: >> >> If |filename| is "subfile,", then proto_len is 7 and the if branch >> copies "file" into proto_str. >> >> If |filename| is "subfile,abcdefg", then proto_len is 7 and the if >> branch copies "file" into proto_str. >> >> If |filename| is "subfile,abcdefg:hijk", then proto_len is 7 and the >> else branch copies "subfile" into proto_str. > > Oh, I see. I was wrong. > >> >> Is this the expected behavior? > > I don't know. However it is the previous behavior, so LGTM. > > Thank's. Applied. Thank's.
diff --git a/libavformat/avio.c b/libavformat/avio.c index 1e79c9dd5c..64248e098b 100644 --- a/libavformat/avio.c +++ b/libavformat/avio.c @@ -263,8 +263,6 @@ static const struct URLProtocol *url_find_protocol(const char *filename) av_strlcpy(proto_str, filename, FFMIN(proto_len + 1, sizeof(proto_str))); - if ((ptr = strchr(proto_str, ','))) - *ptr = '\0'; av_strlcpy(proto_nested, proto_str, sizeof(proto_nested)); if ((ptr = strchr(proto_nested, '+'))) *ptr = '\0';
In url_find_protocol(), proto_str is either "file" or a string consisting of only the characters in URL_SCHEME_CHARS, which does not include ','. Therefore the strchr(proto_str, ',') call always returns NULL. Note: The code was added in commit 6161c41817f6e53abb3021d67ca0f19def682718. Signed-off-by: Wan-Teh Chang <wtc@google.com> --- libavformat/avio.c | 2 -- 1 file changed, 2 deletions(-)