This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Fix readdir_r with long file names
- From: "Carlos O'Donell" <carlos at redhat dot com>
- To: Florian Weimer <fweimer at redhat dot com>
- Cc: libc-alpha at sourceware dot org
- Date: Mon, 10 Jun 2013 10:10:54 -0400
- Subject: Re: [PATCH] Fix readdir_r with long file names
- References: <519220C7 dot 6050705 at redhat dot com> <20130516110136 dot GB11420 at spoyarek dot pnq dot redhat dot com> <5194CDEE dot 4020708 at redhat dot com> <20130516125029 dot GE11420 at spoyarek dot pnq dot redhat dot com> <5194D697 dot 8040106 at redhat dot com> <20130516130952 dot GA16952 at spoyarek dot pnq dot redhat dot com> <519B58EC dot 6060108 at redhat dot com> <51B0A2F9 dot 5060004 at redhat dot com> <51B0B39F dot 4060202 at redhat dot com> <51B0BD36 dot 3030202 at redhat dot com>
On 06/06/2013 12:47 PM, Florian Weimer wrote:
> On 06/06/2013 06:06 PM, Carlos O'Donell wrote:
>> Florian,
>>
>> Thanks for fixing this.
>
> Thanks for the review, Carlos.
>
>> Is there already a test case that tests for this?
>>
>> This seems like the kind of thing we should really be testing.
>
> Most Linux file systems enforce the NAME_MAX limit, so it's difficult
> to trigger. It requires a FUSE file system or a networked file
> system on a system which does not enforce the NAME_MAX limit.
OK, so it falls under the "too complex for the current framework
to support" clause. Thanks for clarifying that. At some point I'll
try to propose some more complex QEMU/VM-based testing strategy
with fault injection to get coverage of all of these failure paths.
>>> @comment POSIX.1
>>> @deftypefun {struct dirent *} readdir (DIR *@var{dirstream})
>>> This function reads the next entry from the directory. It normally
>>> -returns a pointer to a structure containing information about the file.
>>> -This structure is statically allocated and can be rewritten by a
>>> -subsequent call.
>>> +returns a pointer to a structure containing information about the
>>> +file. This structure is associated with the @var{dirstream} handle
>>> +and can be rewritten by a subsequent call.
>>
>> ... "subsequent call with the same handle." ???
>
> On glibc, yes. There are rumors that there used to be systems where
> DIR * was actually an integer file descriptor and readdir used a
> statically allocated buffer. On such systems, readdir_r might
> actually be better than readdir, but on GNU/Linux, there are
> directories where readdir_r fails and readdir succeeds.
OK.
>> Suggest:
>> @code{readdir} is not thread safe. Multiple threads using
>> @code{readdir} on the same @var{dirstream} may overwrite the return
>> value. The use of @code{readdir_r} is a thread-safe alternative,
>> but may suffer from poor portability. If portability is required
>> it is recommended that you use @code{readdir} with external locking.
>
> Here's what I'm proposing instead:
>
> @code{readdir} is not thread safe. In @theglibc{} implementation,
> multiple threads using @code{readdir} on the same @var{dirstream} may
> overwrite the return value. The use of @code{readdir_r} is a
> thread-safe alternative, but may suffer from poor portability. If
> portability is required it is recommended that you use @code{readdir},
> with external locking if multiple threads access the same
> @var{dirstream}.
Looks good to me.
> I followed your other suggestions.
>
>> Please post a new version.
>
> Attached.
>
> --
> Florian Weimer / Red Hat Product Security Team
>
> readdir_r.patch
>
>
> 2013-06-06 Florian Weimer <fweimer@redhat.com>
>
> [BZ #14699]
> * sysdeps/posix/dirstream.h (struct __dirstream): Add errcode
> member.
> * sysdeps/posix/opendir.c (__alloc_dir): Initialize errcode
> member.
> * sysdeps/posix/rewinddir.c (rewinddir): Reset errcode member.
> * sysdeps/posix/readdir_r.c (__READDIR_R): Enforce NAME_MAX limit.
> Return delayed error code. Remove GETDENTS_64BIT_ALIGNED
> conditional.
> * sysdeps/unix/sysv/linux/wordsize-64/readdir_r.c: Do not define
> GETDENTS_64BIT_ALIGNED.
> * sysdeps/unix/sysv/linux/i386/readdir64_r.c: Likewise.
> * manual/filesys.texi (Reading/Closing Directory): Document
> ENAMETOOLONG return value of readdir_r. Recommend readdir more
> strongly.
> * manual/conf.texi (Limits for Files): Add portability note to
> NAME_MAX, PATH_MAX.
> (Pathconf): Add portability note for _PC_NAME_MAX, _PC_PATH_MAX.
This new version looks good to me.
I haven't done thorough technical review, assuming that
the previous long conversation resolved all the technical
issues with the patch.
Cheers,
Carlos.