View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0008423CMakeCMakepublic2009-01-23 08:002009-02-13 07:59
ReporterMartin Apel 
Assigned ToBrad King 
PrioritynormalSeveritymajorReproducibilityalways
StatusclosedResolutionfixed 
PlatformOSOS Version
Product VersionCMake-2-6 
Target VersionFixed in Version 
Summary0008423: GET_FILENAME_COMPONENT (ABSOLUTE) does not resolve trailing symlink
DescriptionOn 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"
Additional InformationChecked with 2.6.3-RC-8 on Linux
TagsNo tags attached.
Attached Filesdiff file icon cmake_fix_bug_8423.diff [^] (3,190 bytes) 2009-01-29 01:27 [Show Content]
c file icon test_realpath.c [^] (506 bytes) 2009-02-06 19:54
diff file icon cmake_get_filename_component_realpath.diff [^] (6,978 bytes) 2009-02-09 02:07 [Show Content]

- Relationships Relation Graph ] Dependency Graph ]

-  Notes
(0014705)
Philip Lowman (developer)
2009-01-29 01:31

Bug is confirmed
Patch is attached against latest CVS with a test case
(0014707)
Bill Hoffman (manager)
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 (manager)
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 (developer)
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 (manager)
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 (manager)
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 (developer)
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 (manager)
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 (developer)
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 (manager)
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 (developer)
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? :)

- Issue History
Date Modified Username Field Change
2009-01-23 08:00 Martin Apel New Issue
2009-01-29 01:27 Philip Lowman File Added: cmake_fix_bug_8423.diff
2009-01-29 01:31 Philip Lowman Note Added: 0014705
2009-01-29 01:31 Philip Lowman Status new => confirmed
2009-01-29 11:39 Bill Hoffman Note Added: 0014707
2009-01-29 11:39 Bill Hoffman Status confirmed => closed
2009-01-29 11:39 Bill Hoffman Resolution open => fixed
2009-02-05 11:53 Brad King Assigned To => Bill Hoffman
2009-02-05 11:53 Brad King Note Added: 0014798
2009-02-05 11:53 Brad King Status closed => feedback
2009-02-05 11:53 Brad King Resolution fixed => reopened
2009-02-06 00:17 Philip Lowman Note Added: 0014803
2009-02-06 08:03 Brad King Note Added: 0014804
2009-02-06 08:35 Brad King Note Added: 0014806
2009-02-06 19:53 Philip Lowman Note Added: 0014815
2009-02-06 19:54 Philip Lowman File Added: test_realpath.c
2009-02-07 16:09 Brad King Note Added: 0014822
2009-02-09 02:06 Philip Lowman Note Added: 0014839
2009-02-09 02:07 Philip Lowman File Added: cmake_get_filename_component_realpath.diff
2009-02-09 09:25 Brad King Note Added: 0014842
2009-02-12 23:41 Philip Lowman Note Added: 0014920
2009-02-13 07:59 Brad King Status feedback => assigned
2009-02-13 07:59 Brad King Assigned To Bill Hoffman => Brad King
2009-02-13 07:59 Brad King Status assigned => closed
2009-02-13 07:59 Brad King Resolution reopened => fixed


Copyright © 2000 - 2017 MantisBT Team
Powered by Mantis Bugtracker