[vlc-devel] [PATCH] Possible buffer overflow in vlc_readdir (fixes #9676)

[Rafaël Carré]

Hello,

On 01/17/14 16:27, Iwao AVE! wrote:
> In vlc_readdir(), the buffer for dirent is too small for a filename with multibyte characters (e.g. utf-8, utf-16, etc.) and causes buffer overflow as explained in the comment.
> 
> This patch replaces readdir_r() with readdir(), but it should be still thread-safe because the directory stream is not shared (see the articles below).
> If using readdir() is not an option, it might be possible to workaround the bug by allocating four bytes for each character.
> 
> http://elliotth.blogspot.jp/2012/10/how-not-to-use-readdirr3.html
> http://womble.decadent.org.uk/readdir_r-advisory.html
> http://udrepper.livejournal.com/18555.html

This makes some sense, but have you checked that callers of vlc_readdir
do not share DIR*
pointer with another thread?

We should check if other libc (BSD, uclibc..) use per-thread buffers though.

AFAIU the function readdir() could have a per-process (e.g. static
state), and in
this case your patch would not be correct.

The articles say this works on glibc, osx, ios, android but it doesn't
talk about the BSD.

Although maybe we can find why _PC_NAME_MAX doesn't return the correct
value:
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=f68e542f3478147986a9c8958942ec649dc06201
is related.

Thanks,

> ---
>  src/posix/filesystem.c | 20 +++-----------------
>  1 file changed, 3 insertions(+), 17 deletions(-)
> 
> diff --git a/src/posix/filesystem.c b/src/posix/filesystem.c
> index b008a6c..2a0496b 100644
> --- a/src/posix/filesystem.c
> +++ b/src/posix/filesystem.c
> @@ -150,28 +150,14 @@ DIR *vlc_opendir (const char *dirname)
>   */
>  char *vlc_readdir( DIR *dir )
>  {
> -    /* Beware that readdir_r() assumes <buf> is large enough to hold the result
> -     * dirent including the file name. A buffer overflow could occur otherwise.
> -     * In particular, pathconf() and _POSIX_NAME_MAX cannot be used here. */
>      struct dirent *ent;
>      char *path = NULL;
>  
> -    long len = fpathconf (dirfd (dir), _PC_NAME_MAX);
> -    /* POSIX says there shall be room for NAME_MAX bytes at all times */
> -    if (len == -1 || len < NAME_MAX)
> -        len = NAME_MAX;
> -    len += sizeof (*ent) + 1 - sizeof (ent->d_name);
> -
> -    struct dirent *buf = malloc (len);
> -    if (unlikely(buf == NULL))
> +    ent = readdir(dir);
> +    if (ent == NULL)
>          return NULL;
> -
> -    int val = readdir_r (dir, buf, &ent);
> -    if (val != 0)
> -        errno = val;
> -    else if (ent != NULL)
> +    else
>          path = strdup (ent->d_name);
> -    free (buf);
>      return path;
>  }
>  
>