[FFmpeg-devel] [PATCH] mov reference files search improvement

[Baptiste Coudurier]

Hi,

On 09/09/2009 11:29 PM, Maksym Veremeyenko wrote:
> Benoit Fouet ???????(??):
>
> [...]
>>> + } else if (type == 0) { // directory name
>>> + av_free(dref->dir);
>>> + dref->dir = av_mallocz(len + 1);
>>
>> why mallocz, and not malloc + dref->dir[len] = 0 ?
>> you're going to overwrite len bytes anyway.
> to make a patch more cosmetic reading *directory name* was done in the
> same way (and a code style too) as *absolute path* (some lines of code
> above), so that is only "traditions honor" (may be *absolute path*
> reading data code submitter knows something more?). Previous patches
> used "malloc + dref->dir[len] = 0"...
>
> So i prepared newer patch that allocate and terminated string...

Changing existing code goes in a separate patch, that's a minor issue.
New code should use malloc + [len] = 0 if Benoit prefers.

> [...]
>
> +static int mov_open_dref(ByteIOContext **pb, char *src, MOVDref *ref)
> +{
> +    if (ref->nlvl_to>  0&&  ref->nlvl_from>  0) {
> +        char filename[1024];
> +        char *src_path;
> +        int c, i, l;
> +
> +        /* find a source dir */
> +        src_path = FFMAX(strrchr(src, '/'), strrchr(src, '\\'));

Why are you checking for straight '\' in src ?

> +        if (src_path)
> +            src_path++;
> +        else
> +            src_path = src;
> +
> +        /* find tail by ref->path and nlvl_To */
> +        for (i = 0, l = strlen(ref->path) - 1; l>= 0; l--)
> +            if ('/' == ref->path[l]) {
> +                if(i == ref->nlvl_to - 1) break;
> +                else                      i++;
> +            }
> +
> +        /* check if it found */
> +        if (i == ref->nlvl_to - 1) {
> +            c = l + 1;
> +
> +            l = 1024;
> +            filename[0] = 0;
> +
> +            strncat(filename, src, src_path - src);
> +            l -= src_path - src;
> +
> +            for (i = 1; i<  ref->nlvl_from; i++, l -= 3)
> +                strncat(filename, "../", l);
> +
> +            strncat(filename, ref->path + c, l);
> +
> +            /* probe open */
> +            if (!url_fopen(pb, filename, URL_RDONLY))
> +                return 0;
> +        }
> +
> +        if (1 == ref->nlvl_to&&  ref->dir) {
> +            l = 1024;
> +            filename[0] = 0;
> +
> +            strncat(filename, src, src_path - src);
> +            l -= src_path - src;
> +
> +            for (i = 1; i<= ref->nlvl_from; i++, l -= 3)
> +                strncat(filename, "../", l);
> +
> +            strncat(filename, ref->dir, l);
> +            l -= strlen(ref->dir);
> +
> +            strncat(filename, "/", l);
> +            l -= 1;
> +
> +            strncat(filename, ref->filename, l);

I feel this can be simplified using snprintf.
Also please use av_strlcat.

> +            /* probe open */
> +            if (!url_fopen(pb, filename, URL_RDONLY))
> +                return 0;
> +        }
> +    }
> +
> +    /* probe open */
> +    return url_fopen(pb, ref->path, URL_RDONLY);

Absolute path must be tried first.

After thinking I'm not sure nlvl_to is worth using. I feel like it's 
either file referenced is in the same directory "./", in dref->dir 
"./<dref->dir", "../"*nlvl_from and maybe "../"*nlvl_from/dref->dir

What do you think ? Did you check how quicktime behave ?

[...]

-- 
Baptiste COUDURIER
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org