MantisBT - CMake
View Issue Details
0008423CMakeCMakepublic2009-01-23 08:002009-02-13 07:59
Martin Apel 
Brad King 
normalmajoralways
closedfixed 
CMake-2-6 
 
0008423: GET_FILENAME_COMPONENT (ABSOLUTE) does not resolve trailing symlink
On Linux a call to GET_FILENAME_COMPONENT with the ABSOLUTE flag does not resolve a symlink, if it is the last component of the path. Example:

On disk the following structure exists in a directory 'foo'
libQtCore.so.4 -> libQtCore.so.4.3.3
libQtCore.so.4.3.3

Then GET_FILENAME_COMPONENT(Output "foo/libQtCore.so.4" ABSOLUTE) results
in "foo/libQtCore.so.4" instead of "foo/libQtCore.so.4.3.3"
Checked with 2.6.3-RC-8 on Linux
No tags attached.
diff cmake_fix_bug_8423.diff (3,190) 2009-01-29 01:27
https://public.kitware.com/Bug/file/2022/cmake_fix_bug_8423.diff
c test_realpath.c (506) 2009-02-06 19:54
https://public.kitware.com/Bug/file/2037/test_realpath.c
diff cmake_get_filename_component_realpath.diff (6,978) 2009-02-09 02:07
https://public.kitware.com/Bug/file/2039/cmake_get_filename_component_realpath.diff
Issue History
2009-01-23 08:00Martin ApelNew Issue
2009-01-29 01:27Philip LowmanFile Added: cmake_fix_bug_8423.diff
2009-01-29 01:31Philip LowmanNote Added: 0014705
2009-01-29 01:31Philip LowmanStatusnew => confirmed
2009-01-29 11:39Bill HoffmanNote Added: 0014707
2009-01-29 11:39Bill HoffmanStatusconfirmed => closed
2009-01-29 11:39Bill HoffmanResolutionopen => fixed
2009-02-05 11:53Brad KingAssigned To => Bill Hoffman
2009-02-05 11:53Brad KingNote Added: 0014798
2009-02-05 11:53Brad KingStatusclosed => feedback
2009-02-05 11:53Brad KingResolutionfixed => reopened
2009-02-06 00:17Philip LowmanNote Added: 0014803
2009-02-06 08:03Brad KingNote Added: 0014804
2009-02-06 08:35Brad KingNote Added: 0014806
2009-02-06 19:53Philip LowmanNote Added: 0014815
2009-02-06 19:54Philip LowmanFile Added: test_realpath.c
2009-02-07 16:09Brad KingNote Added: 0014822
2009-02-09 02:06Philip LowmanNote Added: 0014839
2009-02-09 02:07Philip LowmanFile Added: cmake_get_filename_component_realpath.diff
2009-02-09 09:25Brad KingNote Added: 0014842
2009-02-12 23:41Philip LowmanNote Added: 0014920
2009-02-13 07:59Brad KingStatusfeedback => assigned
2009-02-13 07:59Brad KingAssigned ToBill Hoffman => Brad King
2009-02-13 07:59Brad KingStatusassigned => closed
2009-02-13 07:59Brad KingResolutionreopened => fixed

Notes
(0014705)
Philip Lowman   
2009-01-29 01:31   
Bug is confirmed
Patch is attached against latest CVS with a test case
(0014707)
Bill Hoffman   
2009-01-29 11:39   
/cvsroot/CMake/CMake/Source/cmGetFilenameComponentCommand.cxx,v <-- Source/cmGetFilenameComponentCommand.cxx
new revision: 1.18; previous revision: 1.17
/cvsroot/CMake/CMake/Tests/CMakeTests/CMakeLists.txt,v <-- Tests/CMakeTests/CMakeLists.txt
new revision: 1.6; previous revision: 1.5

Thanks, nice patch, with a test even!
(0014798)
Brad King   
2009-02-05 11:53   
This patch breaks the command when the given path does not exist. The realpath implementations on some UNIX systems (at least AIX and QNX) does not work for non-existing files.
(0014803)
Philip Lowman   
2009-02-06 00:17   
Ok, did some digging and found the root cause of the problem. I uncovered two problems actually.

Firstly, the implementation of SystemTools::Realpath() for Unix has a bug in it. According to the man page for realpath the second parameter (resolved_path) has undefined contents in the event of an error. It is resolved_path that is being assigned unconditionally as the result of the function call and there is no check for a NULL return from realpath().

Not sure what this does on AIX and QNX, it might even segfault the code. On GNU libc under Linux if an invalid path is specified the behavior appears to be rather sane at first glance (which might have been how this got overlooked). In actuality, however, resolved_path is being left modified as one filename/directory past the last valid path (not what we want).

directory exists: /tmp/foo
file does NOT exist: /tmp/foo/1.txt
result of calling Realpath(/tmp/foo/1.txt) is: /tmp/foo/1.txt

directory exists: /tmp
directory does NOT exist: /tmp/foo
result of calling Realpath(/tmp/foo/1.txt) is /tmp/foo

The second bug is that the WIN32 and Linux implementations of Realpath() differ in behavior in the event of an invalid path. WIN32 looks like it will pass back an unmodified (empty) string while Unix passes back the aforementioned undefined string.

I guess first someone has to decide which is the proper behavior before moving forward. If the Unix Realpath() is modified to pass back an empty string in the event of an invalid path (or any of the other errors) there are a couple of places in the code that would need to be changed. E.g. SystemTools::GetPath() and SystemTools::AddKeepPath() might be tweaked. Also cmGetFilenameComponentCommand.cxx should be modified to preserve the existing behavior of not checking the validity of the path specified in GET_FILENAME_COMPONENT().

If on the other hand the proper behavior of Realpath() is to return the original string in the event of an error the Windows implementation would have to change in addition to fixing the glitch in the Unix implementation.
(0014804)
Brad King   
2009-02-06 08:03   
I think the correct fix to the command is to restore the old behavior for compatibility and just update the documentation to say it removes ../ and such but not symlinks. We can create a separate command signature for removing symlinks, and perhaps document that it works only for existing files.

In order to remove symlinks from non-existing paths we would have to strip off one path component at a time until we find something that exists, then realpath it, and then append the removed components.

The SystemTools functions need an overhaul to actually report errno or GetLastError() when something fails, but that is separate from this bug.
(0014806)
Brad King   
2009-02-06 08:35   
I've reverted the original patch and updated the documentation to specify the actual behavior.

/cvsroot/CMake/CMake/Source/cmGetFilenameComponentCommand.cxx,v <-- Source/cmGetFilenameComponentCommand.cxx
new revision: 1.19; previous revision: 1.18
/cvsroot/CMake/CMake/Source/cmGetFilenameComponentCommand.h,v <-- Source/cmGetFilenameComponentCommand.h
new revision: 1.15; previous revision: 1.14
/cvsroot/CMake/CMake/Tests/CMakeTests/CMakeLists.txt,v <-- Tests/CMakeTests/CMakeLists.txt
new revision: 1.7; previous revision: 1.6
/cvsroot/CMake/CMake/Tests/CMakeTests/GetFilenameComponentSymlinksTest.cmake.in,v <-- Tests/CMakeTests/GetFilenameComponentSymlinksTest.cmake.in
new revision: delete; previous revision: 1.1

This is necessary to fix QNX and AIX dashboard submissions.

In order to address the original problem, I suggest creating a new command mode called REALPATH or something like that.
(0014815)
Philip Lowman   
2009-02-06 19:53   
ABSOLUTE did resolve symlinks when the feature was introduced in 2003. You can see this in CVS. cmSystemTools::CollapseFullPath() changed behavior sometime in 2005 causing this regression.

Seeing that it went unnoticed for probably close to 3-4 years I'm gonna have to agree with you that this issue is best solved for now by simply updating the documentation. :)

Regarding the underlying issue, however...

Would it be possible to create a symlink in /tmp and then build and run some (attached) test code on QNX/AIX? I'm not convinced at all that the bug is in their realpath() implementations. I have a sneaky suspicion the reason why realpath() is causing your dashboard to break is because the return value is simply not being checked properly for error before using the resolved_path pointer. As I mentioned earlier, the value of this pointer is undefined if the function errors. QNX/AIX may be just squishing it when the path is invalid, whereas everywhere else we're use getting lucky.


==================================
ABSOLUTE feature introduced:
  else if (args[2] == "ABSOLUTE")
    {
    result = cmSystemTools::CollapseFullPath(filename.c_str());
    }
http://public.kitware.com/cgi-bin/viewcvs.cgi/Source/cmGetFilenameComponentCommand.cxx?revision=1.13&root=CMake&view=markup [^]
==================================
Implementation of CollapseFullPath() had ::realpath() in it (although it has since then been removed).

http://public.kitware.com/cgi-bin/viewcvs.cgi/Source/kwsys/SystemTools.cxx?revision=1.1&root=CMake&view=markup [^]
===================================
(0014822)
Brad King   
2009-02-07 16:09   
I never said anything about the QNX/AIX realpath implementations being buggy, only that they don't work for non-existing files, and I don't think they are documented to work unless the path exists. I also know that the current SystemTools::GetRealPath does not deal with the error condition correctly (and said so in a previous post, but it is a separate bug from the one covered in this issue). This is certainly the reason the dashboard was breaking. I came to the same conclusion before my first post to this bug.

I also know that at one time CollapseFullPath did call realpath and resolve links, but we removed that for various reasons including not working for non-existing files. It's been that way for the 2.4 release which is when most current users probably started with CMake. Therefore we shouldn't change the behavior now.

A full fix to this issue would be a patch that fixes the SystemTools bug and creates a REALPATH mode to get_filename_component.
(0014839)
Philip Lowman   
2009-02-09 02:06   
Is the intent of SystemTools::Realpath() to return the original path passed into the method if realpath() encounters an error? This seems like how the method is used (at least within CMake).

If this is the case, the newly attached patch should work fine (and has the REALPATH) support.
(0014842)
Brad King   
2009-02-09 09:25   
I don't think there was any intent for that case. CMake only called it to collapse the path to its own executables, which always exist. The intent sounds reasonable to me.

Thanks for the patch. I've committed in two pieces:

BUG: Fix GetRealPath when realpath fails
/cvsroot/CMake/CMake/Source/kwsys/SystemTools.cxx,v <-- Source/kwsys/SystemTools.cxx
new revision: 1.233; previous revision: 1.232
/cvsroot/CMake/CMake/Source/kwsys/SystemTools.hxx.in,v <-- Source/kwsys/SystemTools.hxx.in
new revision: 1.76; previous revision: 1.75

ENH: Add get_filename_component(... REALPATH)
/cvsroot/CMake/CMake/Source/cmGetFilenameComponentCommand.cxx,v <-- Source/cmGetFilenameComponentCommand.cxx
new revision: 1.20; previous revision: 1.19
/cvsroot/CMake/CMake/Source/cmGetFilenameComponentCommand.h,v <-- Source/cmGetFilenameComponentCommand.h
new revision: 1.16; previous revision: 1.15
/cvsroot/CMake/CMake/Tests/CMakeTests/CMakeLists.txt,v <-- Tests/CMakeTests/CMakeLists.txt
new revision: 1.8; previous revision: 1.7
/cvsroot/CMake/CMake/Tests/CMakeTests/GetFilenameComponentRealpathTest.cmake.in,v <-- Tests/CMakeTests/GetFilenameComponentRealpathTest.cmake.in
initial revision: 1.1
(0014920)
Philip Lowman   
2009-02-12 23:41   
Thanks for committing the fix. Sorry for the regression, by the way. SystemTools::Realpath() seemed to work fine when I called it so I didn't look that closely at the implementation until after Brad mentioned the dashboard breaking.

I think this bug is assigned to Bill and is in some need for resolving? :)