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

[Rémi Denis-Courmont]

   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