Hi Ingo,
I have made an alternative patch that solves the problem for Windows. I fear it
is a bit hackish (or whackish for Windows) and probably not a good solution. I
have not tested it on Linux either.
Unfortunately, I have also found more unusual behavior on Windows. Not only does
the my_stat() on Windows not accept the ending '/' in the path passed as a
parameter, you can no longer set @@global.backupdir = 'some/valid/path' without
the ending '/' on Windows (see attached diff for ancillary changes to other
tests. That is simply weird.
This has led me to the conclusion that we cannot make the code do what the bug
report has described as a solution. That saddens me because your observations
and suggested improvements have merit.
Thus, I think unless someone else has another idea other than the one I describe
below, we will have to mark this one as 'not a bug' and make sure the
documentation contains the (now) limitations described in the bug report.
Jorgen: do you concur?
Chuck
> Sigh. If there was only a way to build and test faster on Windows, I'd
> test even trivial patches there.
I feel your pain...everyday. ;)
...
> my_access() checks directories as well as files and other file system
> objects. We use it with the flags F_OK|W_OK. So my_access() checks if
> the filesystem object exists and is writable. But it does not care if it
> is a directory, a file, or something else.
Aha, I see.
> The intended purpose of the test_if_directory() method was to test if a
> file system object was a directory or not a directory. The method was
> designed to return TRUE if the path name referenced an existent file
> system object and that this object was a directory. It returned FALSE,
> if the path referenced a non-existent object, or an existent object,
> which was not a directory. My idea was, that this check would make the
> behavior of the backupdir variable more intuitive. It would accept its
> value only if it is an existent directory and refuse all other cases as
> invalid.
Right. Well, we can make this work -- just not using my_stat(). We could take
the time to create a Windows specific 'is_a_directory()' method, but that would
mean some very specific code added to the server.
...
> I respect your decision. Just in case that you are curious, what my idea
> was behind test_if_directory(), I'll try to explain. Somehow I start to
> feel like repeating myself. Why am I not able to make even trivial
> things clear? (Sigh!) Starting from my personal preference, to refuse
> non-directory paths, I thought that it would be a great idea to be able
> to tell, if a given path references a directory or not. Since I didn't
> find any such function in mysys, I planned to create one. The function
> should return TRUE if the path references a directory and FALSE if it
> does not. I remembered the nice system service stat(2), which returns a
> bunch of information about a path. Among other information, it returns
> the type of object. This is encoded in the st_mode element of the
> returned structure. There is a type mask S_IFTYPE and a type value
> S_IFDIR. The convenience macro S_ISDIR combines these values to return
> zero or non-zero, depending on the type of the object. MY_S_ISDIR is
> just a redefine. my_stat() returns a pointer to the information struct
> if the filesystem object exists and the information could be retrieved,
> and NULL otherwise. Using the above information, test_if_directory()
> returns TRUE or FALSE, depending on the fact if the path references an
> existent directory or not. I hope, this is somewhat believable now.
Thanks for the explanation. I think I could create a Windows-specific method to
test this but it would mean we would be adding to the platform-specific code
dilemma.
Chuck
=== modified file 'include/my_dir.h'
--- include/my_dir.h 2008-06-20 00:27:48 +0000
+++ include/my_dir.h 2009-03-30 15:40:11 +0000
@@ -100,6 +100,7 @@ extern MY_DIR *my_dir(const char *path,m
extern void my_dirend(MY_DIR *buffer);
extern MY_STAT *my_stat(const char *path, MY_STAT *stat_area, myf my_flags);
extern int my_fstat(int filenr, MY_STAT *stat_area, myf MyFlags);
+extern my_bool test_if_directory(const char* path);
#endif /* MY_DIR_H */
=== modified file 'mysql-test/suite/backup/r/backup_backupdir.result'
--- mysql-test/suite/backup/r/backup_backupdir.result 2009-03-18 22:01:12 +0000
+++ mysql-test/suite/backup/r/backup_backupdir.result 2009-03-30 20:15:13 +0000
@@ -7,7 +7,7 @@ CREATE TABLE bup_backupdir.t1(a INT);
INSERT INTO bup_backupdir.t1 VALUES (1), (2), (3);
Create a directory for backup images
Reset backupdir
-SET @@global.backupdir = '../../tmp/backup';
+SET @@global.backupdir = '../../tmp/backup/';
Perform backup
BACKUP DATABASE bup_backupdir TO 'bup_backupdir1.bak';
backup_id
@@ -76,6 +76,49 @@ Warnings:
Warning #### The path specified for the system variable backupdir cannot be accessed or
is invalid. ref: This_is_really_stupid/not/there/at/all
Warning #### The path specified for the system variable backupdir cannot be accessed or
is invalid. ref: This_is_really_stupid/not/there/at/all
set global max_allowed_packet=DEFAULT;
+#
+# Set backupdir to an existent file.
+#
+# Create file.
+# Set backupdir.
+SET @@global.backupdir = '../../tmp/backup_file.txt';
+Warnings:
+Warning 1739 The path specified for the system variable backupdir cannot be accessed or
is invalid. ref: ../../tmp/backup_file.txt
+# Attempt backup to a pure file name in invalid backupdir.
+BACKUP DATABASE bup_backupdir TO 'bup_backupdir_x.bak';
+ERROR HY000: The path specified for the system variable backupdir cannot be accessed or
is invalid. ref: ../../tmp/backup_file.txt
+# Attempt restore from a pure file name in invalid backupdir.
+RESTORE FROM 'bup_backupdir_x.bak' OVERWRITE;
+ERROR HY000: The path specified for the system variable backupdir cannot be accessed or
is invalid. ref: ../../tmp/backup_file.txt
+# Attempt backup to a relative path name in invalid backupdir.
+# Try 'test' in case current directory is used instead of backupdir.
+BACKUP DATABASE bup_backupdir TO 'test/bup_backupdir_x.bak';
+ERROR HY000: The path specified for the system variable backupdir cannot be accessed or
is invalid. ref: ../../tmp/backup_file.txt
+# Attempt restore from a relative path name in invalid backupdir.
+# Try 'test' in case current directory is used instead of backupdir.
+RESTORE FROM 'test/bup_backupdir_x.bak' OVERWRITE;
+ERROR HY000: The path specified for the system variable backupdir cannot be accessed or
is invalid. ref: ../../tmp/backup_file.txt
+# Attempt backup to a backref path in invalid backupdir.
+BACKUP DATABASE bup_backupdir TO '../bup_backupdir_x.bak';
+ERROR HY000: The path specified for the system variable backupdir cannot be accessed or
is invalid. ref: ../../tmp/backup_file.txt
+# Attempt restore from a backref path in invalid backupdir.
+RESTORE FROM '../bup_backupdir_x.bak' OVERWRITE;
+ERROR HY000: The path specified for the system variable backupdir cannot be accessed or
is invalid. ref: ../../tmp/backup_file.txt
+# Attempt backup to an absolute path.
+# *Actual BACKUP DATABASE command not printed due to
+# non-deterministic path.
+# *Performing:
+# *BACKUP DATABASE bup_backupdir TO
+# '$MYSQLTEST_VARDIR/bup_backupdir_x.bak';
+ERROR HY000: The path specified for the system variable backupdir cannot be accessed or
is invalid. ref: ../../tmp/backup_file.txt
+# Ensure backup image file does not exist
+# Attempt restore from an absolute path.
+# *Actual RESTORE command not printed due to
+# non-deterministic path.
+# *Performing:
+# *RESTORE FROM
+# '$MYSQLTEST_VARDIR/bup_backupdir_x.bak';
+ERROR HY000: The path specified for the system variable backupdir cannot be accessed or
is invalid. ref: ../../tmp/backup_file.txt
Cleanup
Reset backupdir
SET @@global.backupdir = @@global.datadir;
=== modified file 'mysql-test/suite/backup/r/backup_securebackup.result'
--- mysql-test/suite/backup/r/backup_securebackup.result 2009-02-09 18:17:55 +0000
+++ mysql-test/suite/backup/r/backup_securebackup.result 2009-03-30 20:55:33 +0000
@@ -24,7 +24,7 @@ Ensure backup image file went to the cor
Change backupdir to securebackup_path/subpath
(MYSQLD_DATADIR/securebackup_path/subpath)
-SET @@global.backupdir = 'securebackup_path/subpath';
+SET @@global.backupdir = 'securebackup_path/subpath/';
Backup to subpath of path specified by --secure-backup-file-priv option,
no dir in backup file name
@@ -62,7 +62,7 @@ ERROR HY000: The MySQL server is running
Change backupdir to securebackup_path/subpath
(MYSQLD_DATADIR/securefilepriv_path/subpath)
-SET @@global.backupdir = 'securefilepriv_path/subpath';
+SET @@global.backupdir = 'securefilepriv_path/subpath/';
(MYSQLD_DATADIR/securefilepriv_path)
BACKUP DATABASE mysqltest TO 'securefilepriv_path/bup_sfp5.bak';
ERROR HY000: The MySQL server is running with the --secure-backup-file-priv option so it
cannot execute this statement
=== modified file 'mysql-test/suite/backup/t/backup_backupdir.test'
--- mysql-test/suite/backup/t/backup_backupdir.test 2009-02-26 22:19:09 +0000
+++ mysql-test/suite/backup/t/backup_backupdir.test 2009-03-30 20:12:28 +0000
@@ -37,7 +37,7 @@ rmdir $MYSQLTEST_VARDIR/tmp/backup;
mkdir $MYSQLTEST_VARDIR/tmp/backup;
--echo Reset backupdir
-SET @@global.backupdir = '../../tmp/backup';
+SET @@global.backupdir = '../../tmp/backup/';
--echo Perform backup
--replace_column 1 #
@@ -135,6 +135,75 @@ SET @@global.backupdir = 'This_is_really
set global max_allowed_packet=DEFAULT;
+--echo #
+--echo # Set backupdir to an existent file.
+--echo #
+
+--echo # Create file.
+--error 0,1
+--remove_file $MYSQLTEST_VARDIR/tmp/backup_file.txt
+--write_file $MYSQLTEST_VARDIR/tmp/backup_file.txt EOF
+blabla
+EOF
+--echo # Set backupdir.
+SET @@global.backupdir = '../../tmp/backup_file.txt';
+
+--echo # Attempt backup to a pure file name in invalid backupdir.
+--replace_result $MYSQLTEST_VARDIR MYSQLTEST_VARDIR
+--error ER_BACKUP_BACKUPDIR
+BACKUP DATABASE bup_backupdir TO 'bup_backupdir_x.bak';
+
+--echo # Attempt restore from a pure file name in invalid backupdir.
+--replace_result $MYSQLTEST_VARDIR MYSQLTEST_VARDIR
+--error ER_BACKUP_BACKUPDIR
+RESTORE FROM 'bup_backupdir_x.bak' OVERWRITE;
+
+--echo # Attempt backup to a relative path name in invalid backupdir.
+--echo # Try 'test' in case current directory is used instead of backupdir.
+--replace_result $MYSQLTEST_VARDIR MYSQLTEST_VARDIR
+--error ER_BACKUP_BACKUPDIR
+BACKUP DATABASE bup_backupdir TO 'test/bup_backupdir_x.bak';
+
+--echo # Attempt restore from a relative path name in invalid backupdir.
+--echo # Try 'test' in case current directory is used instead of backupdir.
+--replace_result $MYSQLTEST_VARDIR MYSQLTEST_VARDIR
+--error ER_BACKUP_BACKUPDIR
+RESTORE FROM 'test/bup_backupdir_x.bak' OVERWRITE;
+
+--echo # Attempt backup to a backref path in invalid backupdir.
+--error ER_BACKUP_BACKUPDIR
+BACKUP DATABASE bup_backupdir TO '../bup_backupdir_x.bak';
+
+--echo # Attempt restore from a backref path in invalid backupdir.
+--error ER_BACKUP_BACKUPDIR
+RESTORE FROM '../bup_backupdir_x.bak' OVERWRITE;
+
+--echo # Attempt backup to an absolute path.
+--echo # *Actual BACKUP DATABASE command not printed due to
+--echo # non-deterministic path.
+--echo # *Performing:
+--echo # *BACKUP DATABASE bup_backupdir TO
+--echo # '\$MYSQLTEST_VARDIR/bup_backupdir_x.bak';
+--disable_query_log
+--error ER_BACKUP_BACKUPDIR
+eval BACKUP DATABASE bup_backupdir TO '$MYSQLTEST_VARDIR/bup_backupdir_x.bak';
+--enable_query_log
+
+--echo # Ensure backup image file does not exist
+--error 1
+--file_exists $MYSQLTEST_VARDIR/bup_backupdir_x.bak
+
+--echo # Attempt restore from an absolute path.
+--echo # *Actual RESTORE command not printed due to
+--echo # non-deterministic path.
+--echo # *Performing:
+--echo # *RESTORE FROM
+--echo # '\$MYSQLTEST_VARDIR/bup_backupdir_x.bak';
+--disable_query_log
+--error ER_BACKUP_BACKUPDIR
+eval RESTORE FROM '$MYSQLTEST_VARDIR/bup_backupdir_x.bak' OVERWRITE;
+--enable_query_log
+
--echo Cleanup
--echo Reset backupdir
=== modified file 'mysql-test/suite/backup/t/backup_securebackup.test'
--- mysql-test/suite/backup/t/backup_securebackup.test 2009-02-09 18:17:55 +0000
+++ mysql-test/suite/backup/t/backup_securebackup.test 2009-03-30 20:55:01 +0000
@@ -71,7 +71,7 @@ BACKUP DATABASE mysqltest TO 'secureback
--echo
--echo Change backupdir to securebackup_path/subpath
--echo (MYSQLD_DATADIR/securebackup_path/subpath)
-SET @@global.backupdir = 'securebackup_path/subpath';
+SET @@global.backupdir = 'securebackup_path/subpath/';
--echo
--echo Backup to subpath of path specified by --secure-backup-file-priv option,
@@ -141,7 +141,7 @@ RESTORE FROM 'securefilepriv_path/bup_sf
--echo
--echo Change backupdir to securebackup_path/subpath
--echo (MYSQLD_DATADIR/securefilepriv_path/subpath)
-SET @@global.backupdir = 'securefilepriv_path/subpath';
+SET @@global.backupdir = 'securefilepriv_path/subpath/';
#
# Now check to ensure backup cannot write to the --secure-file-priv location even
=== modified file 'mysys/my_lib.c'
--- mysys/my_lib.c 2008-06-17 23:37:23 +0000
+++ mysys/my_lib.c 2009-03-30 16:24:27 +0000
@@ -508,6 +508,41 @@ error:
#endif /* _WIN32 */
+
+/**
+ Test if path is a directory.
+
+ @param[in] path path name
+
+ @return test result
+ @retval TRUE path is directory
+ @retval FALSE path is not directory
+*/
+
+my_bool test_if_directory(const char* path)
+{
+ MY_STAT statbuf;
+ char dir_path[FN_REFLEN];
+ int i= strlen(path);
+ strnmov(dir_path, path, i);
+
+#if defined(_WIN32)
+ /*
+ If this is a Windows machine, we need to strip the last / or \ off the
+ path if present so that my_stat() can correct identify the path
+ as containing a directory.
+ */
+ if ((dir_path[i-1] == '/') || (dir_path[i-1] == '\\'))
+ dir_path[i-1] = 0;
+#endif /* _WIN32 */
+
+ if (my_stat(dir_path, &statbuf, MYF(0)) &&
+ MY_S_ISDIR(statbuf.st_mode))
+ return TRUE;
+ return FALSE;
+}
+
+
/****************************************************************************
** File status
** Note that MY_STAT is assumed to be same as struct stat
=== modified file 'sql/backup/kernel.cc'
--- sql/backup/kernel.cc 2009-03-16 14:38:05 +0000
+++ sql/backup/kernel.cc 2009-03-30 15:40:12 +0000
@@ -61,6 +61,7 @@
#include "../mysql_priv.h"
#include "../si_objects.h"
+#include "my_dir.h"
#include "backup_kernel.h"
#include "backup_info.h"
@@ -160,8 +161,15 @@ execute_backup_command(THD *thd,
Check backupdir for validity. This is needed since we cannot trust
that the path is still valid. Access could have changed or the
folders in the path could have been moved, deleted, etc.
- */
- if (backupdir->length() && my_access(backupdir->c_ptr(), (F_OK|W_OK)))
+ This can of course happen after the check too...
+ It has been decided that this check shall be done even if the
+ BACKUP/RESTORE statement contains an absolute path name.
+ If this requirement is dropped one day, we can easily add
+ && !test_if_hard_path(lex->backup_dir.str). See Bug#43536.
+ */
+ if (backupdir->length() &&
+ (!test_if_directory(backupdir->c_ptr_safe()) ||
+ my_access(backupdir->c_ptr_safe(), (F_OK|W_OK))))
DBUG_RETURN(send_error(context, ER_BACKUP_BACKUPDIR, backupdir->c_ptr()));
switch (lex->sql_command) {
=== modified file 'sql/set_var.cc'
--- sql/set_var.cc 2009-03-18 21:09:40 +0000
+++ sql/set_var.cc 2009-03-30 15:40:12 +0000
@@ -3032,11 +3032,9 @@ static int sys_check_backupdir(THD *thd,
if (!path_length)
return 0;
- /*
- Check if directory exists and we have permission to create file and
- write to file.
- */
- if (my_access(path, (F_OK|W_OK)))
+ /* Check if backupdir is a directory and we have write permission to it. */
+ if (!test_if_directory(path) ||
+ my_access(path, (F_OK|W_OK)))
goto err;
return 0;
@@ -3108,7 +3106,9 @@ static bool sys_update_backupdir(THD *th
logger.unlock();
pthread_mutex_unlock(&LOCK_global_system_variables);
- if (my_access(sys_var_backupdir.value, (F_OK|W_OK)))
+ /* Check if backupdir is a directory and we have write permission to it. */
+ if (!test_if_directory(sys_var_backupdir.value) ||
+ my_access(sys_var_backupdir.value, (F_OK|W_OK)))
goto err;
return result;
