Re: glibc: new patch to fix *_name_split() and callers

[Jérémie Koenig]

2010/6/14 Jérémie Koenig <jk@jk.fr.eu.org>:
> I'll test the patch tomorrow and post the results,

So, here they are. I attach the test program I used and its output
under Debian eglibc 2.11.1-3 with and without the patch applied, as
well as under Linux.

Short version: not so bad, but it seems EINVAL should generally be
filtered after the fact instead of trying to prevent it, because the
translator could fail with EOPNOSUPP instead (ie., it's a file) and we
often need this information.

There would still be a "trailing slash on file" problem for those
functions which use directory_name_split(), though.


Legend: C = current, P = patched, L = Linux
"file" is a regular file, "dir" is a directory and "nosuch" does not exist.

=== bind(), uses file_name_split()

CP.  afunix_bind(""): No such file or directory
..L  afunix_bind(""): Success

This corresponds to "abstract" addresses, I'm not sure Hurd supports
that or whether we're supposed to handle addrlen !=
sizeof(sockaddr_un) in this case.

CP.  afunix_bind("/"): Permission denied
..L  afunix_bind("/"): Address already in use

I believe Linux is right.

CP.  afunix_bind("file/"): Not a directory
..L  afunix_bind("file/"): Address already in use

I believe we are.

C..  afunix_bind("dir/"): Invalid argument
.PL  afunix_bind("dir/"): Address already in use

Fixed by the patch as expected

=== unlink(), uses directory_name_split()

C..  unlink("/"): Invalid argument
.P.  unlink("/"): Operation not permitted

Fixed by the patch as expected.

CP.  unlink("dir"): Operation not permitted
CP.  unlink("dir/"): Operation not permitted

As it should be.

CP.  unlink("file/"): Success
..L  unlink("file/"): Not a directory

Needs to be fixed in the translators.

..L  unlink("/"): Is a directory
..L  unlink("dir"): Is a directory
..L  unlink("dir/"): Is a directory

For what it's worth; well-known divergence from POSIX.

=== mkdir() and rmdir(), use directory_name_split()

C..  mkdir("/"): Invalid argument
C..  rmdir("/"): Invalid argument
C..  symlink("/"): Permission denied
.PL  mkdir("/"): File exists
.PL  rmdir("/"): Device or resource busy
.PL  symlink("/"): File exists

Fixed by the patch as expected.

=== symlink(), uses file_name_split()

CPL  symlink("nosuch/"): No such file or directory
C..  symlink("dir/"): Invalid argument (dir_link("") says so)
C..  symlink("file/"): Not a directory (dir_link unsupported)
.PL  symlink("dir/"): File exists
.PL  symlink("file/"): File exists

I believe we should either:
  * have symlink("file/") fail with ENOTDIR or
  * have symlink("nosuch/") succeed.

I would go for #1 by handling EINVAL from the translator, but #2 would
be possible by using directory_name_split() I think.

=== link(), uses file_name_split() for the target

C..  link("file", "/"): Invalid argument (dir_link("") as for symlink)
C..  link("file", "dir/"): Invalid argument
.PL  link("file", "/"): File exists
.PL  link("file", "dir/"): File exists

Fixed by the patch as expected.

CP.  link("/", *): Device or resource busy
CP.  link("dir", "file"): Is a directory
CP.  link("dir/", "file"): Is a directory
CP.  link("dir", "dir"): Is a directory
CP.  link("dir/", "dir"): Is a directory
CP.  link("dir", "nosuch"): Is a directory
CP.  link("dir/", "nosuch"): Is a directory
C..  link("dir", "/"): Is a directory
C..  link("dir/", "/"): Is a directory
C..  link("dir", "dir/"): Is a directory
C..  link("dir/", "dir/"): Is a directory
C..  link("dir", "file/"): Is a directory
C..  link("dir/", "file/"): Is a directory

This is from the translator. Should be EPERM instead.

.PL  link("dir", "/"): File exists
.PL  link("dir/", "/"): File exists
.PL  link("dir", "dir/"): File exists
.PL  link("dir/", "dir/"): File exists

The patch short-circuits those because of the target's trailing slash,
but letting it fail with EPERM would be okay. Furthermore,

.PL  link("file", "file/"): File exists
.PL  link("dir", "file/"): File exists
.PL  link("dir/", "file/"): File exists

here it's totally inappropriate, whereas

C..  link("file", "file/"): Not a directory

was good enough.

=== rename(), uses directory_name_split(), twice

C..  careful_rename("/", *): Invalid argument
C..  careful_rename(*, "/"): Invalid argument, except for:
C..   careful_rename("nosuch", "/"): No such file or directory
C..   careful_rename("nosuch/", "/"): No such file or directory
C..   careful_rename("dir", "/"): ext2fs crashes badly
C..   careful_rename("dir/", "/"): likewise
.PL  careful_rename("/", *): Device or resource busy
.PL  careful_rename(*, "/"): Device or resource busy

Fixed, changed and mitigated to some extent by the patch, respectively.

CP.  careful_rename("file", "file/"): Success
..L  careful_rename("file", "file/"): Not a directory

Still broken.

CP.  careful_rename("file", "dir/"): Is a directory
..L  careful_rename("file", "dir/"): Not a directory

We get it right.

CP.  careful_rename("file", "nosuch/"): Success
..L  careful_rename("file", "nosuch/"): Not a directory

They get it right.

CP.  careful_rename("file/", "file"): Success
CP.  careful_rename("file/", "file/"): Success
CP.  careful_rename("file/", "dir"): Is a directory
CP.  careful_rename("file/", "dir/"): Is a directory
CP.  careful_rename("file/", "nosuch"): Success
CP.  careful_rename("file/", "nosuch/"): Success
..L  careful_rename("file/", "file"): Not a directory
..L  careful_rename("file/", "file/"): Not a directory
..L  careful_rename("file/", "dir"): Not a directory
..L  careful_rename("file/", "dir/"): Not a directory
..L  careful_rename("file/", "nosuch"): Not a directory
..L  careful_rename("file/", "nosuch/"): Not a directory

They get it right.

My head hurts,
-- 
Jérémie Koenig <jk@jk.fr.eu.org>
http://jk.fr.eu.org/

summary.txt
Description: Text document

test-link-calls.tar.gz
Description: GNU Zip compressed data

results
Description: Binary data

results_patched
Description: Binary data

results_linux
Description: Binary data