rtmp: Don't assume path points to a string of nonzero length

Message ID 1337158179-49136-1-git-send-email-martin@martin.st
State Superseded
Headers show

Commit Message

Martin Storsjö May 16, 2012, 8:49 a.m.
If using the new -rtmp_app and -rtmp_playpath parameters,
one can in many cases set the main url to just rtmp://server/.
If the trailing slash is omitted, path is a string of zero length,
and using path+1 will end up reading uninitialized data.
---
 libavformat/rtmpproto.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Diego Elio Pettenò May 16, 2012, 9:04 a.m. | #1
Il 16/05/2012 01:49, Martin Storsjö ha scritto:
> -        char *p = strchr(path + 1, '/');
> +        char *p = *path ? strchr(path + 1, '/') : NULL;
>          if (!p) {
> -            fname = path + 1;
> +            fname = *path ? path + 1 : path;

Wouldn't it be more readable to consider the special case path == '\0'
even before testing for it to be "/ondemand", instead of using the
ternary twice?

It would also avoid one call to strncmp(), shallow as that is.
Martin Storsjö May 16, 2012, 9:07 a.m. | #2
On Wed, 16 May 2012, Diego Elio Pettenò wrote:

> Il 16/05/2012 01:49, Martin Storsjö ha scritto:
>> -        char *p = strchr(path + 1, '/');
>> +        char *p = *path ? strchr(path + 1, '/') : NULL;
>>          if (!p) {
>> -            fname = path + 1;
>> +            fname = *path ? path + 1 : path;
>
> Wouldn't it be more readable to consider the special case path == '\0'
> even before testing for it to be "/ondemand", instead of using the
> ternary twice?
>
> It would also avoid one call to strncmp(), shallow as that is.

I guess that's also an option. In that case, we more explicitly state that 
as a special condition, while this version keeps the same logic and code 
flow, just avoids reads outside of strings.

But if others think the same, I can change it.

// Martin
Diego Elio Pettenò May 16, 2012, 9:11 a.m. | #3
Il 16/05/2012 02:07, Martin Storsjö ha scritto:
> 
> I guess that's also an option. In that case, we more explicitly state
> that as a special condition, while this version keeps the same logic and
> code flow, just avoids reads outside of strings.

I see your point, I just dislike using the ternary operator twice in
three rows with the same condition — mostly because if they were to
depart the logic of it could be lost.

But it's not a strong feeling, so yes, let's see if someone else prefers
one or the other.

Patch

diff --git a/libavformat/rtmpproto.c b/libavformat/rtmpproto.c
index 427655c..7b31492 100644
--- a/libavformat/rtmpproto.c
+++ b/libavformat/rtmpproto.c
@@ -895,9 +895,9 @@  static int rtmp_open(URLContext *s, const char *uri, int flags)
         fname = path + 10;
         memcpy(rt->app, "ondemand", 9);
     } else {
-        char *p = strchr(path + 1, '/');
+        char *p = *path ? strchr(path + 1, '/') : NULL;
         if (!p) {
-            fname = path + 1;
+            fname = *path ? path + 1 : path;
             rt->app[0] = '\0';
         } else {
             char *c = strchr(p + 1, ':');