|
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? :) |
|
cmake_fix_bug_8423.diff (3,190) 2009-01-29 01:27
test_realpath.c (506) 2009-02-06 19:54