| Bug #43767 | Wrong path if backupdir is symlink and backref path used | ||
|---|---|---|---|
| Submitted: | 20 Mar 2009 11:47 | Modified: | 29 Jun 2009 14:40 |
| Reporter: | Ingo Strüwing | Email Updates: | |
| Status: | Closed | Impact on me: | |
| Category: | MySQL Server: Backup | Severity: | S2 (Serious) |
| Version: | 6.0 | OS: | Any |
| Assigned to: | Ingo Strüwing | CPU Architecture: | Any |
[20 Mar 2009 11:47]
Ingo Strüwing
[20 Mar 2009 11:51]
Ingo Strüwing
Suggested triage values: Defect: Medium. Annoying, but not really serious. Workaround: None. Impact: Substantial, assuming that use of symbolic links is common, but the use of ../ is less common.
[27 Mar 2009 16:34]
Ingo Strüwing
If this will be fixed, please note that some tests need to have their workarounds removed. The tests are not pushed yet. They are developed for WL#4518 (Online Backup: Test backupdir variable). The affected test cases are: backup_backupdir, backup_backupdir_not_windows, and backup_backupdir_sync.
[24 Apr 2009 14:55]
Ingo Strüwing
This is fixed together with Bug#43747 (BACKUP with backref path succeeds on invalid backupdir). See there for more information.
[24 Apr 2009 14:58]
Bugs System
A patch for this bug has been committed. After review, it may be pushed to the relevant source trees for release in the next version. You can access the patch from: http://lists.mysql.com/commits/72789 2797 Ingo Struewing 2009-04-24 Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used A backupdir variable value like 'existent_file.txt' and a backup image file name like '../backup_file.bak' allowed for a successful BACKUP and RESTORE (Bug #43747). It had been decided that we do not want to allow BACKUP or RESTORE to succeed, if backupdir is set to an invalid path (compare Bug 43536 - BACKUP with absolute path fails on invalid backupdir). A backupdir variable value like 'symlink_to_dir1_dir2' and a backup image file name like '../backup_file.bak' located the file in the wrong directory. If "symlink_to_dir1_dir2 -> dir1/dir2", the used file was just "backup_file.bak". Cleanup of "../" was done based on the symlink itself, not based on its target (dir2). Normal file system operations interpret a path of "symlink_to_dir1_dir2/../backup_file.bak" as "dir1/backup_file.bak". The main problem was that MySQL did not have a path handling function, which cleans a path while respecting symbolic links, works even if the path name references a non-existent file system object, and works on Windows too. For more details see the explanation in the bug report #43747. Another problem was that the backupdir variable was checked for an existent and writable file system object only. It was not required that this object needs to be a directory. The main part of the patch is the new function safe_cleanup_cat_path() in mf_pack.c. Another important new function is test_if_directory() in my_lib.c. It allows to verify that backupdir is not just a writable path, but also a directory. Yet another new function is cut_print_path() in mf_format.c. It supports the creation of good error messages. Many error message templates have limited space for path names, e.g. %.64s. Often the last part of a path name is more informative for error messages than its begin. This function returns a cut path with a prepended "... ", if the path is longer than the length limit. Yet another new function is is_usable_directory() in set_var.cc. It supports checking of variable values when they are set. To avoid frequent use of strlen() I added length variables to the seldom or never changed variables home_dir, mysql_real_data_home, and opt_secure_backup_file_priv. Finally I used these functions in the backup kernel to build an absolute, clean path for the backupdir to check it for being a writable directory and to prepend it to a relative backup image file path. Additionally I used the functions on opt_secure_backup_file_priv before comparing it against the resulting backup image file path. To have a good test coverage for the new non-trivial function safe_cleanup_cat_path(), I made a unittest for it. @ include/my_dir.h Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Added declaration for test_if_directory(). @ include/my_sys.h Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Added declarations for home_dir_len, cut_print_path(), and cleanup_cat_path(). @ mysql-test/suite/backup/r/backup_backupdir.result Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Updated test result. @ mysql-test/suite/backup/r/backup_backupdir_not_windows.result Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used New test result. @ mysql-test/suite/backup/r/backup_securebackup.result Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Updated test result. @ mysql-test/suite/backup/t/backup_backupdir.test Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Added proper cleanup for backupdir. Added SELECTs for backupdir to show its changes. Removed tests for limited length of the variable. Made the test for a "really long string" better checkable. Added new tests for the reported bug. Added tests for special variable values (NULL and DEFAULT). @ mysql-test/suite/backup/t/backup_backupdir_not_windows.test Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used New test case for Bug#43767. @ mysql-test/suite/backup/t/backup_securebackup-master.opt Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Added a --backupdir option for coverage testing. @ mysql-test/suite/backup/t/backup_securebackup.test Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Added proper cleanup for backupdir. Fixed wrong tests (bup_sfp5.bak, bup_sfp1.bak). Added a printout for the backupdir option. @ mysys/mf_format.c Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Added definition of new function cut_print_path(). @ mysys/mf_pack.c Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Added definition of new function safe_cleanup_cat_path(). @ mysys/my_init.c Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Added setup for home_dir_len. @ mysys/my_lib.c Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Added definition of new function test_if_directory(). @ mysys/my_static.c Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Added declaration of home_dir_len. @ sql/backup/kernel.cc Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Moved check of backupdir from execute_backup_command() to Backup_restore_ctx::prepare_path(). Changed the latter to use safe_cleanup_cat_path() and check_backupdir(). @ sql/backup/stream.cc Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Changed Stream::test_secure_file_priv_access() to use safe_cleanup_cat_path() on opt_secure_backup_file_priv. @ sql/mysql_priv.h Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Added declarations of mysql_real_data_home_len and opt_secure_backup_file_priv_len. @ sql/mysqld.cc Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Added definitions and setup of mysql_real_data_home_len and opt_secure_backup_file_priv_len. Moved check of backupdir value from mysqld_get_one_option() to fix_paths(). Used is_usable_directory() on backupdir and opt_secure_backup_file_priv. @ sql/set_var.cc Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Removed double declarations. Removed sys_check_backupdir() altogether. All checks are done in sys_update_backupdir() now. Changed make_default_backupdir() to default_backupdir(), which allocates the value. Added definition of new function check_backupdir(). Added definition of new function is_usable_directory(). Changed sys_update_backupdir() to use is_usable_directory(). @ sql/set_var.h Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Added declarations for is_usable_directory(), check_backupdir(), and default_backupdir(). @ sql/share/errmsg.txt Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Changed ER_BACKUP_BACKUPDIR to the more genaral usable ER_NOT_RW_DIR. @ unittest/mysys/CMakeLists.txt Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Added safe_cleanup_cat_path-t. @ unittest/mysys/Makefile.am Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Added safe_cleanup_cat_path-t. @ unittest/mysys/safe_cleanup_cat_path-t.c Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used New unit test for safe_cleanup_cat_path().
[28 Apr 2009 17:04]
Chuck Bell
Patch approved. Well done.
[29 Apr 2009 9:11]
Jørgen Løland
Good to push.
[1 May 2009 12:15]
Ingo Strüwing
Queued to 6.0-backup.
[6 May 2009 16:56]
Ingo Strüwing
I'm going to modify the solution in that path cleanup for ../ will not only be suppressed if the preceding path element is a symbolic link, but also if it is not a directory. The current solution will do the following wrong: dir/non-existent-file.txt/../existent-file.txt This will result in dir/existent-file.txt, which is not what standard file operations would do.
[8 May 2009 15:56]
Bugs System
A patch for this bug has been committed. After review, it may be pushed to the relevant source trees for release in the next version. You can access the patch from: http://lists.mysql.com/commits/73665 2808 Ingo Struewing 2009-05-08 Bug#43767 - Wrong path if backupdir is symlink and backref path used Addendum fix to Bug#43747 (BACKUP with backref path succeeds on invalid backupdir) and Bug#43767. The former fix checked every path element for being a symlink. Only then it was prevented to let ../ remove this path element. But the correct solution is to prevent ../ from removing all path elements except of real (non-symlinked) directories. A path like dir/non-existent-file.txt/../existent-file.txt must not be cleaned up to dir/existent-file.txt. Normal (operating system) file operations don't do that either. ".." is a valid path element with its back-referencing semantics in an existent directory only. @ include/my_sys.h Bug#43767 - Wrong path if backupdir is symlink and backref path used Added define for MY_RETURN_SYMLINK. @ mysys/mf_pack.c Bug#43767 - Wrong path if backupdir is symlink and backref path used Changed safe_cleanup_cat_path() to call test_if_real_directory() instead of my_is_symlink() on each path element. Fixed two issues with removal of ./ and duplicate / behind non-real-dirctory path elements. @ mysys/my_lib.c Bug#43767 - Wrong path if backupdir is symlink and backref path used Added function test_if_real_directory(). @ unittest/mysys/safe_cleanup_cat_path-t.c Bug#43767 - Wrong path if backupdir is symlink and backref path used Added directory creation/removal. Reworked tests to show when directories and non-directories are used. Added tests so that we use a directory and a non-directory everywhere it makes sense.
[8 May 2009 15:58]
Ingo Strüwing
Please review the small addendum patch.
[19 May 2009 9:49]
Bugs System
A patch for this bug has been committed. After review, it may be pushed to the relevant source trees for release in the next version. You can access the patch from: http://lists.mysql.com/commits/74470 2808 Ingo Struewing 2009-05-19 Bug#43767 - Wrong path if backupdir is symlink and backref path used Addendum fix to Bug#43747 (BACKUP with backref path succeeds on invalid backupdir) and Bug#43767. The former fix checked every path element for being a symlink. Only then it was prevented to let ../ remove this path element. But the correct solution is to prevent ../ from removing all path elements except of real (non-symlinked) directories. A path like dir/non-existent-file.txt/../existent-file.txt must not be cleaned up to dir/existent-file.txt. Normal (operating system) file operations don't do that either. ".." is a valid path element with its back-referencing semantics in an existent directory only. However, on Windows, we need to do the clean-ups irrespectively of the file system object. Windows system services do the clean-up even after path elements that refer to non-existent objects. In the above example, dir/existent-file.txt is used. safe_cleanup_cat_path(), its unittest, and the regression tests do now respect the platform difference. @ include/my_sys.h Bug#43767 - Wrong path if backupdir is symlink and backref path used Added MY_RETURN_SYMLINK. @ mysql-test/suite/backup/include/backup_image_path.inc Bug#43767 - Wrong path if backupdir is symlink and backref path used New body for new test cases. @ mysql-test/suite/backup/r/backup_image_path_nonwin.result Bug#43767 - Wrong path if backupdir is symlink and backref path used New test result. @ mysql-test/suite/backup/r/backup_image_path_win.result Bug#43767 - Wrong path if backupdir is symlink and backref path used New test result. @ mysql-test/suite/backup/t/backup_image_path_nonwin.test Bug#43767 - Wrong path if backupdir is symlink and backref path used New test case. @ mysql-test/suite/backup/t/backup_image_path_win.test Bug#43767 - Wrong path if backupdir is symlink and backref path used New test case. @ mysys/mf_pack.c Bug#43767 - Wrong path if backupdir is symlink and backref path used Fixed comments. Changed check for symlink to check for real directory. @ mysys/my_lib.c Bug#43767 - Wrong path if backupdir is symlink and backref path used Added test_if_real_directory(). @ unittest/mysys/safe_cleanup_cat_path-t.c Bug#43767 - Wrong path if backupdir is symlink and backref path used Changed unittest to work with real directories. Added tests to be more systematic. Grouped platform specific tests.
[20 May 2009 17:22]
Chuck Bell
Approved
[21 May 2009 8:18]
Jørgen Løland
Good to push
[22 May 2009 14:53]
Ingo Strüwing
Queued to 6.0-backup.
[29 Jun 2009 14:40]
Paul DuBois
No changelog entry needed. Not in any released version.
[6 Jan 2010 17:55]
Bugs System
A patch for this bug has been committed. After review, it may be pushed to the relevant source trees for release in the next version. You can access the patch from: http://lists.mysql.com/commits/96163 3035 Ingo Struewing 2010-01-06 WL#5101 - MySQL Backup back port Merged revid:ingo.struewing@sun.com-20090424145553-xmtiynisc12yf3wl Merged revid:ingo.struewing@sun.com-20090519094845-d1a0tsqervmfwp57 Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used A backupdir variable value like 'existent_file.txt' and a backup image file name like '../backup_file.bak' allowed for a successful BACKUP and RESTORE (Bug #43747). It had been decided that we do not want to allow BACKUP or RESTORE to succeed, if backupdir is set to an invalid path (compare Bug 43536 - BACKUP with absolute path fails on invalid backupdir). A backupdir variable value like 'symlink_to_dir1_dir2' and a backup image file name like '../backup_file.bak' located the file in the wrong directory. If "symlink_to_dir1_dir2 -> dir1/dir2", the used file was just "backup_file.bak". Cleanup of "../" was done based on the symlink itself, not based on its target (dir2). Normal file system operations interpret a path of "symlink_to_dir1_dir2/../backup_file.bak" as "dir1/backup_file.bak". The main problem was that MySQL did not have a path handling function, which cleans a path while respecting symbolic links, works even if the path name references a non-existent file system object, and works on Windows too. For more details see the explanation in the bug report #43747. Another problem was that the backupdir variable was checked for an existent and writable file system object only. It was not required that this object needs to be a directory. The main part of the patch is the new function safe_cleanup_cat_path() in mf_pack.c. Another important new function is test_if_directory() in my_lib.c. It allows to verify that backupdir is not just a writable path, but also a directory. Yet another new function is cut_print_path() in mf_format.c. It supports the creation of good error messages. Many error message templates have limited space for path names, e.g. %.64s. Often the last part of a path name is more informative for error messages than its begin. This function returns a cut path with a prepended "... ", if the path is longer than the length limit. Yet another new function is is_usable_directory() in set_var.cc. It supports checking of variable values when they are set. To avoid frequent use of strlen() I added length variables to the seldom or never changed variables home_dir, mysql_real_data_home, and opt_secure_backup_file_priv. Finally I used these functions in the backup kernel to build an absolute, clean path for the backupdir to check it for being a writable directory and to prepend it to a relative backup image file path. Additionally I used the functions on opt_secure_backup_file_priv before comparing it against the resulting backup image file path. To have a good test coverage for the new non-trivial function safe_cleanup_cat_path(), I made a unittest for it. --- Bug#43767 - Wrong path if backupdir is symlink and backref path used Addendum fix to Bug#43747 (BACKUP with backref path succeeds on invalid backupdir) and Bug#43767. The former fix checked every path element for being a symlink. Only then it was prevented to let ../ remove this path element. But the correct solution is to prevent ../ from removing all path elements except of real (non-symlinked) directories. A path like dir/non-existent-file.txt/../existent-file.txt must not be cleaned up to dir/existent-file.txt. Normal (operating system) file operations don't do that either. ".." is a valid path element with its back-referencing semantics in an existent directory only. However, on Windows, we need to do the clean-ups irrespectively of the file system object. Windows system services do the clean-up even after path elements that refer to non-existent objects. In the above example, dir/existent-file.txt is used. safe_cleanup_cat_path(), its unittest, and the regression tests do now respect the platform difference. original changeset: 2599.146.1 original changeset: 2599.148.1 @ include/my_sys.h WL#5101 - MySQL Backup back port Removed a MY_FLAG value clash. @ libmysql/CMakeLists.txt WL#5101 - MySQL Backup back port Added mysys as include directory. Required for my_rnd.c. @ unittest/mysys/CMakeLists.txt WL#5101 - MySQL Backup back port Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Added safe_cleanup_cat_path-t. @ unittest/mysys/Makefile.am WL#5101 - MySQL Backup back port Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used Added safe_cleanup_cat_path-t. @ unittest/mysys/safe_cleanup_cat_path-t.c WL#5101 - MySQL Backup back port Bug#43747 - BACKUP with backref path succeeds on invalid backupdir Bug#43767 - Wrong path if backupdir is symlink and backref path used New unit test for safe_cleanup_cat_path().
