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

[Iwao AVE!]

Thank you both for reviewing my patch!

Reading Rémi's post, I guess I need to clarify the problem first.
I am not a C expert, so please bear with me about my primitive methods.

>> In vlc_readdir(), the buffer for dirent is too small for a filename with
>> multibyte characters (e.g. utf-8, utf-16, etc.)
>
> How is it too small? fpathconf() returns a value in BYTES, not in
> CHARACTERS. Thus the character encoding in general and use of Unicode in
> particular is irrelevant. (For the reference, the current code assumes
> UTF-8 is used.) With that in mind, I fail to see how the current
> computation could underflow.

While narrowing down the problem, I simply added a printf as follows:

    long len = fpathconf (dirfd (dir), _PC_NAME_MAX);
    printf("len = %d\n", len);

and it always prints 255 on Mac OS X 10.8 and 10.9.
But, of course, I can create a file with 255 Japanese characters as its name.
Some bytes are added to the result and 280 is passed to malloc(), but
it's still too small.

Please let me know if there is anything I should try.

Thank you for your time,
Iwao


2014/1/18 Rémi Denis-Courmont <remi at remlab.net>:
>    Hello,
>
> On Sat, 18 Jan 2014 00:27:21 +0900, Iwao AVE! <harawata at gmail.com> wrote:
>> In vlc_readdir(), the buffer for dirent is too small for a filename with
>> multibyte characters (e.g. utf-8, utf-16, etc.)
>
> How is it too small? fpathconf() returns a value in BYTES, not in
> CHARACTERS. Thus the character encoding in general and use of Unicode in
> particular is irrelevant. (For the reference, the current code assumes
> UTF-8 is used.) With that in mind, I fail to see how the current
> computation could underflow.
>
>> and causes buffer overflow as explained in the comment.
>
> Again, I do not see anything wrong with the current computation.
>
>> This patch replaces readdir_r() with readdir(), but it should be still
>> thread-safe because the directory stream is not shared.
>
> I would agree that reading the same open directory handle from multiple
> threads simultaneously is not indispensible. But...
>
>> If using readdir() is not an option, it might be possible to workaround
>> the bug by allocating four bytes for each character.
>
> I am not convinced at this point that there is a bug surrounding
> readdir_r() within the VLC code base. I concede that readdir_r() is poorly
> designed (not me!) and hard to use correctly, but I still fail to see a
> bug.
>
>> 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
>
> We have vlc_readdir() precisely because readdir_r() is hard to use
> correctly. If readdir_r() is not needed (or really unfixable), then
> vlc_readdir() should be removed *completely*. It would simplify the calling
> code quite a bit - no need for free() any longer.
>
> --
> Rémi Denis-Courmont
> Sent from my collocated server