|
From: | Jérémie Koenig |
Subject: | Re: glibc: new patch to fix *_name_split() and callers |
Date: | Wed, 16 Jun 2010 04:10:27 +0200 |
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
[Prev in Thread] | Current Thread | [Next in Thread] |