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

 
 

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

Submitter:  Emilio Pozuelo Monfort <pochu>
Submitted:  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:  pochu Originator Name: 
Open/Closed:  Closed Reproducibility:  Every Time
Size (loc):  None Planned Release:  None
Effort:  0.00
Wiki-like text discussion box: 


* Mandatory Fields

Add a New Comment Rich Markup
   

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>
Group 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>
Group Member
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>
Group Member
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:

    * item1 (foo): ...
    * item2 (bar): ...


Carl Fredrik Hammar <hammy>
Group 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>
Group Member
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>
Group Member
Sat 24 Apr 2010 08:04:10 PM UTC, comment #1: 
Emilio Pozuelo Monfort <pochu>
Group Member
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>
Group Member

 

(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)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    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.13-cf05.
    Corresponding source code