bugThe GNU Hurd - Bugs: bug #29655, linkat() fails because...

 
 

bug #29655: linkat() fails because __file_name_lookup_at() problems

Submitted by:  Emilio Pozuelo Monfort <pochu>
Submitted on:  Sat 24 Apr 2010 10:50:52 AM UTC  
 
Category:  glibc Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Standard Compliance
Status:  Fixed Privacy:  Public
Assigned to:  Emilio Pozuelo Monfort <pochu> Originator Name: 
Open/Closed:  Closed Reproducibility:  Every Time
Size (loc):  None Planned Release:  None
Effort:  0.00
Wiki-like text discussion box: 

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Thu 08 Jul 2010 12:56:49 PM UTC, comment #7:

The glibc patch has been committed, so I think we can close this issue?

Thomas Schwinge <tschwinge>
Project Administrator
Wed 02 Jun 2010 11:48:03 AM UTC, comment #6:

I've fixed the changelog and submitted it:

http://sourceware.org/ml/libc-alpha/2010-06/msg00000.html

Emilio Pozuelo Monfort <pochu>
Project MemberIn charge of this item.
Tue 25 May 2010 10:57:51 AM UTC, comment #5:

As we discussed last week on #hurd, the patch looks good and should be submitted to libc-alpha after I have the copyright assignment for glibc on file (and after updating the changelog).

Emilio Pozuelo Monfort <pochu>
Project MemberIn charge of this item.
Sat 08 May 2010 08:39:57 PM UTC, comment #4:

This has already been said on IRC but I might as well repeat it here:
POSIX.1-2008 says that only `AT_SYMLINK_FOLLOW` is applicable to linkat()
(see http://www.opengroup.org/onlinepubs/9699919799/functions/linkat.html).
I would recommend testing it in `__file_name_lookup_at()` as I described
earlier. Otherwise, you'd need to check it in all `at` functions
except `openat()', because they should fail when they're given
`AT_SYMLINK_FOLLOW`.

You are a bit too verbose in the ChangeLog, the first item is fine,
but the second item should just be "Updated comment." or some such.
In the third item, you shouldn't mention motivation, i.e "Pass O_NOLINK
in flags" would be enough. Such information should be placed in code
comments, as it already is. Also there should be no blank lines between
related items, and they are all related:

Carl Fredrik Hammar <hammy>
Project Member
Sat 08 May 2010 10:37:28 AM UTC, comment #3:

Thanks for the review.

This patch (hopefully) clarifies the comment.

I don't understand your second point though. How can "linkat(..., AT_SYMLINK_FOLLOW)" be wrong? Do you mean that in that case with my patch, file_name_lookup_at() will have O_NOLINK in flags and AT_SYMLINK_FOLLOW in at_flags, which is a bit contradictory, and that I should only pass O_NOLINK in linkat() if AT_SYMLINK_FOLLOW is not in its flags argument?

(file #20480)

Emilio Pozuelo Monfort <pochu>
Project MemberIn charge of this item.
Tue 27 Apr 2010 04:37:54 PM UTC, comment #2:

The patch looks good, but I do have two minor suggestions.

First, I found the comment a bit confusing. The "default" behavior is
determined by the O_NOLINK flag, and isn't something that is constant.
So perhaps you could change it to mention this, or better yet explain
the flags in terms of setting or clearing O_NOLINK.

Second, it is good that you check that both AT_SYMLINK_FOLLOW and
AT_SYMLINK_NOFOLLOW aren't set, but it is still possible to call `at`
functions with the wrong flag, e.g. `linkat(..., AT_SYMLINK_FOLLOW)`.
You could fix this by only allowing AT_SYMLINK_NOFOLLOW if O_NOLINK is
not set and AT_SYMLINK_FOLLOW if it is set.

Carl Fredrik Hammar <hammy>
Project Member
Sat 24 Apr 2010 08:04:10 PM UTC, comment #1:

Return value submitted at
http://sources.redhat.com/ml/libc-alpha/2010-04/msg00046.html

Emilio Pozuelo Monfort <pochu>
Project MemberIn charge of this item.
Sat 24 Apr 2010 10:50:52 AM UTC, original submission:

While looking at the coreutils test suite failures, I found that the new ln fails miserably for hard links:

pochu@strauss:~$ touch a && /home/pochu/coreutils-8.4/src/ln a b
/home/pochu/coreutils-8.4/src/ln: creating hard link `b' => `a': Computer bought the farm

Samuel helped me to debug the problem and found this:

00:09 < youpi> one difference is that the newer ln uses linkat
00:20 < youpi> seems like the AT_SYMLINK_FOLLOW is the culprit, making __file_name_lookup_at return -1 with errno == EINVAL
00:20 < youpi> while __file_name_lookup is supposed to return a port, not -1
00:20 < youpi> resulting to a bogus error handling
00:24 < youpi> it seems to me like the logic in the hurd port is inverted: susv says that by default linkat shouldn't dereference symlinks, i.e. the flags passed by linkat should be O_NOLINK, but if.
AT_SYMLINK_FOLLOW is given, it should be cleared in __file_name_lookup_at()
00:24 < youpi> pochu: could you try this out?
00:25 < youpi> and of course, btw, the EINVAL case should be fixed
00:26 < youpi> (using _hurd_fail is really completely bogus here)
00:27 < youpi> (just setting errno to EINVAL and returning MACH_PORT_NULL should be fine)
00:35 < pochu> youpi: so, __file_name_lookup_at() is returning -1 (bug), which is then passed to __file_getlinknode(), which sets errno to EMACH_SEND_INVALID_DEST, and __hurd_fail() change it to EIEIO?
00:35 < youpi> probably yes

He was right. I first tested a one liner to return MACH_PORT_NULL instead of -1 in __file_name_lookup_at(),

- return __hurd_fail (EINVAL);
+ return (__hurd_fail (EINVAL), MACH_PORT_NULL);

and with that ln failed with invalid argument:

pochu@strauss:~/coreutils-8.4/src$ LD_LIBRARY_PATH=/home/pochu/lib ./ln /tmp/a /tmp/b
./ln: creating hard link `/tmp/b' => `/tmp/a': Invalid argument

And then I tried to fix the AT_SYMLINK_FOLLOW, and the attached patch make ln work fine and the failing tests to pass.

Basically, _file_name_lookup_at() doesn't allow other AT flags than AT_SYMLINK_NOFOLLOW, and it doesn't default to not follow links, as Samuel said, see http://www.opengroup.org/onlinepubs/9699919799/functions/linkat.html:
"If the AT_SYMLINK_FOLLOW flag is clear in the flag argument and the path1 argument names a symbolic link, a new link is created for the symbolic link path1 and not its target."

Emilio Pozuelo Monfort <pochu>
Project MemberIn charge of this item.

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #20480:  Fix-linkat-on-Hurd2.patch added by pochu (2KiB - application/octet-stream)
file #20309:  glibc-ln2.patch added by pochu (2KiB - application/octet-stream - Fix linkat() and __file_name_lookup_at())

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by sthibaul (Updated the item)
  • -email is unavailable- added by tschwinge (Posted a comment)
  • -email is unavailable- added by hammy (Posted a comment)
  • -email is unavailable- added by pochu (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2010-07-08 sthibaul StatusNone => Fixed
        Open/ClosedOpen => Closed
    2010-06-02 pochu Item GroupNone => Standard Compliance
        Assigned toNone => pochu
        ReproducibilityNone => Every Time
    2010-05-25 pochu Wiki-like text discussion boxAs we have just discussed on #hurd, the patch looks good and should be submitted to libc-alpha after I have the copyright assignment for glibc on file (and after updating the changelog). =>
    2010-05-12 pochu Wiki-like text discussion box => As we have just discussed on #hurd, the patch looks good and should be submitted to libc-alpha after I have the copyright assignment for glibc on file (and after updating the changelog).
    2010-05-08 pochu Attached File- => Added Fix-linkat-on-Hurd2.patch, #20480
    2010-04-24 pochu Attached File- => Added glibc-ln2.patch, #20309

    Back to the top


    Powered by Savane 3.3