Skip to content

Commit d72dbb4

Browse files
vuvovavuvova
committed
bugfix: remove my_delete_with_symlink()
it was race condition prone. instead use either a pair of my_delete() calls with already resolved paths, or a safe high-level function my_handler_delete_with_symlink(), like MyISAM and Aria already do.
1 parent 955f2f0 commit d72dbb4

File tree

7 files changed

+62
-121
lines changed

7 files changed

+62
-121
lines changed

‎include/my_sys.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -578,7 +578,6 @@ extern int my_realpath(char *to, const char *filename, myf MyFlags);
578578
extern File my_create_with_symlink(const char *linkname, const char *filename,
579579
int createflags, int access_flags,
580580
myf MyFlags);
581-
extern int my_delete_with_symlink(const char *name, myf MyFlags);
582581
extern int my_rename_with_symlink(const char *from,const char *to,myf MyFlags);
583582
extern int my_symlink(const char *content, const char *linkname, myf MyFlags);
584583
extern int my_handler_delete_with_symlink(PSI_file_key key, const char *name,

‎include/mysql/psi/mysql_file.h

Lines changed: 0 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -434,20 +434,6 @@
434434
inline_mysql_file_create_with_symlink(P1, P2, P3, P4, P5)
435435
#endif
436436

437-
/**
438-
@def mysql_file_delete_with_symlink(K, P1, P2)
439-
Instrumented delete with symbolic link.
440-
@c mysql_file_delete_with_symlink is a replacement
441-
for @c my_delete_with_symlink.
442-
*/
443-
#ifdef HAVE_PSI_INTERFACE
444-
#define mysql_file_delete_with_symlink(K, P1, P2) \
445-
inline_mysql_file_delete_with_symlink(K, __FILE__, __LINE__, P1, P2)
446-
#else
447-
#define mysql_file_delete_with_symlink(K, P1, P2) \
448-
inline_mysql_file_delete_with_symlink(P1, P2)
449-
#endif
450-
451437
/**
452438
@def mysql_file_rename_with_symlink(K, P1, P2, P3)
453439
Instrumented rename with symbolic link.
@@ -1348,33 +1334,6 @@ inline_mysql_file_create_with_symlink(
13481334
return file;
13491335
}
13501336

1351-
static inline int
1352-
inline_mysql_file_delete_with_symlink(
1353-
#ifdef HAVE_PSI_INTERFACE
1354-
PSI_file_key key, const char *src_file, uint src_line,
1355-
#endif
1356-
const char *name, myf flags)
1357-
{
1358-
int result;
1359-
#ifdef HAVE_PSI_INTERFACE
1360-
struct PSI_file_locker *locker= NULL;
1361-
PSI_file_locker_state state;
1362-
if (likely(PSI_server != NULL))
1363-
{
1364-
locker= PSI_server->get_thread_file_name_locker(&state, key, PSI_FILE_DELETE,
1365-
name, &locker);
1366-
if (likely(locker != NULL))
1367-
PSI_server->start_file_wait(locker, (size_t) 0, src_file, src_line);
1368-
}
1369-
#endif
1370-
result= my_delete_with_symlink(name, flags);
1371-
#ifdef HAVE_PSI_INTERFACE
1372-
if (likely(locker != NULL))
1373-
PSI_server->end_file_wait(locker, (size_t) 0);
1374-
#endif
1375-
return result;
1376-
}
1377-
13781337
static inline int
13791338
inline_mysql_file_rename_with_symlink(
13801339
#ifdef HAVE_PSI_INTERFACE

‎mysys/my_symlink2.c

Lines changed: 0 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -91,27 +91,6 @@ File my_create_with_symlink(const char *linkname, const char *filename,
9191
DBUG_RETURN(file);
9292
}
9393

94-
/*
95-
If the file was a symlink, delete both symlink and the file which the
96-
symlink pointed to.
97-
*/
98-
99-
int my_delete_with_symlink(const char *name, myf MyFlags)
100-
{
101-
char link_name[FN_REFLEN];
102-
int was_symlink= (!my_disable_symlinks &&
103-
!my_readlink(link_name, name, MYF(0)));
104-
int result;
105-
DBUG_ENTER("my_delete_with_symlink");
106-
107-
if (!(result=my_delete(name, MyFlags)))
108-
{
109-
if (was_symlink)
110-
result=my_delete(link_name, MyFlags);
111-
}
112-
DBUG_RETURN(result);
113-
}
114-
11594
/*
11695
If the file is a normal file, just rename it.
11796
If the file is a symlink:

‎sql/handler.cc

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3378,12 +3378,10 @@ int handler::delete_table(const char *name)
33783378
int saved_error= 0;
33793379
int error= 0;
33803380
int enoent_or_zero= ENOENT; // Error if no file was deleted
3381-
char buff[FN_REFLEN];
33823381

33833382
for (const char **ext=bas_ext(); *ext ; ext++)
33843383
{
3385-
fn_format(buff, name, "", *ext, MY_UNPACK_FILENAME|MY_APPEND_EXT);
3386-
if (mysql_file_delete_with_symlink(key_file_misc, buff, MYF(0)))
3384+
if (my_handler_delete_with_symlink(key_file_misc, name, *ext, 0))
33873385
{
33883386
if (my_errno != ENOENT)
33893387
{

‎sql/sql_db.cc

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1083,9 +1083,9 @@ static bool find_db_tables_and_rm_known_files(THD *thd, MY_DIR *dirp,
10831083
strxmov(filePath, path, "/", file->name, NullS);
10841084
/*
10851085
We ignore ENOENT error in order to skip files that was deleted
1086-
by concurrently running statement like REAPIR TABLE ...
1086+
by concurrently running statement like REPAIR TABLE ...
10871087
*/
1088-
if (my_delete_with_symlink(filePath, MYF(0)) &&
1088+
if (my_handler_delete_with_symlink(key_file_misc, filePath, "", MYF(0)) &&
10891089
my_errno != ENOENT)
10901090
{
10911091
my_error(EE_DELETE, MYF(0), filePath, my_errno);
@@ -1206,7 +1206,7 @@ long mysql_rm_arc_files(THD *thd, MY_DIR *dirp, const char *org_path)
12061206
continue;
12071207
}
12081208
strxmov(filePath, org_path, "/", file->name, NullS);
1209-
if (mysql_file_delete_with_symlink(key_file_misc, filePath, MYF(MY_WME)))
1209+
if (my_handler_delete_with_symlink(key_file_misc, filePath, "", MYF(MY_WME)))
12101210
{
12111211
goto err;
12121212
}

‎storage/maria/ma_create.c

Lines changed: 30 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,8 @@ int maria_create(const char *name, enum data_file_type datafile_type,
5252
unique_key_parts,fulltext_keys,offset, not_block_record_extra_length;
5353
uint max_field_lengths, extra_header_size, column_nr;
5454
ulong reclength, real_reclength,min_pack_length;
55-
char filename[FN_REFLEN], linkname[FN_REFLEN], *linkname_ptr;
55+
char kfilename[FN_REFLEN], klinkname[FN_REFLEN], *klinkname_ptr;
56+
char dfilename[FN_REFLEN], dlinkname[FN_REFLEN], *dlinkname_ptr;
5657
ulong pack_reclength;
5758
ulonglong tot_length,max_rows, tmp;
5859
enum en_fieldtype type;
@@ -805,19 +806,19 @@ int maria_create(const char *name, enum data_file_type datafile_type,
805806
/* chop off the table name, tempory tables use generated name */
806807
if ((path= strrchr(ci->index_file_name, FN_LIBCHAR)))
807808
*path= '\0';
808-
fn_format(filename, name, ci->index_file_name, MARIA_NAME_IEXT,
809+
fn_format(kfilename, name, ci->index_file_name, MARIA_NAME_IEXT,
809810
MY_REPLACE_DIR | MY_UNPACK_FILENAME |
810811
MY_RETURN_REAL_PATH | MY_APPEND_EXT);
811812
}
812813
else
813814
{
814-
fn_format(filename, ci->index_file_name, "", MARIA_NAME_IEXT,
815+
fn_format(kfilename, ci->index_file_name, "", MARIA_NAME_IEXT,
815816
MY_UNPACK_FILENAME | MY_RETURN_REAL_PATH |
816817
(have_iext ? MY_REPLACE_EXT : MY_APPEND_EXT));
817818
}
818-
fn_format(linkname, name, "", MARIA_NAME_IEXT,
819+
fn_format(klinkname, name, "", MARIA_NAME_IEXT,
819820
MY_UNPACK_FILENAME|MY_APPEND_EXT);
820-
linkname_ptr= linkname;
821+
klinkname_ptr= klinkname;
821822
/*
822823
Don't create the table if the link or file exists to ensure that one
823824
doesn't accidently destroy another table.
@@ -831,10 +832,10 @@ int maria_create(const char *name, enum data_file_type datafile_type,
831832
{
832833
char *iext= strrchr(name, '.');
833834
int have_iext= iext && !strcmp(iext, MARIA_NAME_IEXT);
834-
fn_format(filename, name, "", MARIA_NAME_IEXT,
835+
fn_format(kfilename, name, "", MARIA_NAME_IEXT,
835836
MY_UNPACK_FILENAME | MY_RETURN_REAL_PATH |
836837
(have_iext ? MY_REPLACE_EXT : MY_APPEND_EXT));
837-
linkname_ptr= NullS;
838+
klinkname_ptr= NullS;
838839
/*
839840
Replace the current file.
840841
Don't sync dir now if the data file has the same path.
@@ -854,7 +855,7 @@ int maria_create(const char *name, enum data_file_type datafile_type,
854855
NOTE: The filename is compared against unique_file_name of every
855856
open table. Hence we need a real path here.
856857
*/
857-
if (_ma_test_if_reopen(filename))
858+
if (_ma_test_if_reopen(kfilename))
858859
{
859860
my_printf_error(HA_ERR_TABLE_EXIST, "Aria table '%s' is in use "
860861
"(most likely by a MERGE table). Try FLUSH TABLES.",
@@ -863,8 +864,8 @@ int maria_create(const char *name, enum data_file_type datafile_type,
863864
goto err;
864865
}
865866

866-
if ((file= mysql_file_create_with_symlink(key_file_kfile, linkname_ptr,
867-
filename, 0, create_mode,
867+
if ((file= mysql_file_create_with_symlink(key_file_kfile, klinkname_ptr,
868+
kfilename, 0, create_mode,
868869
MYF(MY_WME|create_flag))) < 0)
869870
goto err;
870871
errpos=1;
@@ -1118,30 +1119,30 @@ int maria_create(const char *name, enum data_file_type datafile_type,
11181119
/* chop off the table name, tempory tables use generated name */
11191120
if ((path= strrchr(ci->data_file_name, FN_LIBCHAR)))
11201121
*path= '\0';
1121-
fn_format(filename, name, ci->data_file_name, MARIA_NAME_DEXT,
1122+
fn_format(dfilename, name, ci->data_file_name, MARIA_NAME_DEXT,
11221123
MY_REPLACE_DIR | MY_UNPACK_FILENAME | MY_APPEND_EXT);
11231124
}
11241125
else
11251126
{
1126-
fn_format(filename, ci->data_file_name, "", MARIA_NAME_DEXT,
1127+
fn_format(dfilename, ci->data_file_name, "", MARIA_NAME_DEXT,
11271128
MY_UNPACK_FILENAME |
11281129
(have_dext ? MY_REPLACE_EXT : MY_APPEND_EXT));
11291130
}
1130-
fn_format(linkname, name, "",MARIA_NAME_DEXT,
1131+
fn_format(dlinkname, name, "",MARIA_NAME_DEXT,
11311132
MY_UNPACK_FILENAME | MY_APPEND_EXT);
1132-
linkname_ptr= linkname;
1133+
dlinkname_ptr= dlinkname;
11331134
create_flag=0;
11341135
}
11351136
else
11361137
{
1137-
fn_format(filename,name,"", MARIA_NAME_DEXT,
1138+
fn_format(dfilename,name,"", MARIA_NAME_DEXT,
11381139
MY_UNPACK_FILENAME | MY_APPEND_EXT);
1139-
linkname_ptr= NullS;
1140+
dlinkname_ptr= NullS;
11401141
create_flag= (flags & HA_CREATE_KEEP_FILES) ? 0 : MY_DELETE_OLD;
11411142
}
11421143
if ((dfile=
1143-
mysql_file_create_with_symlink(key_file_dfile, linkname_ptr,
1144-
filename, 0, create_mode,
1144+
mysql_file_create_with_symlink(key_file_dfile, dlinkname_ptr,
1145+
dfilename, 0, create_mode,
11451146
MYF(MY_WME | create_flag | sync_dir))) < 0)
11461147
goto err;
11471148
errpos=3;
@@ -1189,19 +1190,21 @@ int maria_create(const char *name, enum data_file_type datafile_type,
11891190
mysql_file_close(dfile, MYF(0));
11901191
/* fall through */
11911192
case 2:
1192-
if (! (flags & HA_DONT_TOUCH_DATA))
1193-
mysql_file_delete_with_symlink(key_file_dfile,
1194-
fn_format(filename,name,"",MARIA_NAME_DEXT,
1195-
MY_UNPACK_FILENAME | MY_APPEND_EXT),
1196-
sync_dir);
1193+
if (! (flags & HA_DONT_TOUCH_DATA))
1194+
{
1195+
mysql_file_delete(key_file_dfile, dfilename, MYF(sync_dir));
1196+
if (dlinkname_ptr)
1197+
mysql_file_delete(key_file_dfile, dlinkname_ptr, MYF(sync_dir));
1198+
}
11971199
/* fall through */
11981200
case 1:
11991201
mysql_file_close(file, MYF(0));
12001202
if (! (flags & HA_DONT_TOUCH_DATA))
1201-
mysql_file_delete_with_symlink(key_file_kfile,
1202-
fn_format(filename,name,"",MARIA_NAME_IEXT,
1203-
MY_UNPACK_FILENAME | MY_APPEND_EXT),
1204-
sync_dir);
1203+
{
1204+
mysql_file_delete(key_file_kfile, kfilename, MYF(sync_dir));
1205+
if (klinkname_ptr)
1206+
mysql_file_delete(key_file_kfile, klinkname_ptr, MYF(sync_dir));
1207+
}
12051208
}
12061209
my_free(log_data);
12071210
my_free(rec_per_key_part);

‎storage/myisam/mi_create.c

Lines changed: 28 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,8 @@ int mi_create(const char *name,uint keys,MI_KEYDEF *keydefs,
4545
max_key_block_length,unique_key_parts,fulltext_keys,offset;
4646
uint aligned_key_start, block_length, res;
4747
ulong reclength, real_reclength,min_pack_length;
48-
char filename[FN_REFLEN],linkname[FN_REFLEN], *linkname_ptr;
48+
char kfilename[FN_REFLEN],klinkname[FN_REFLEN], *klinkname_ptr;
49+
char dfilename[FN_REFLEN],dlinkname[FN_REFLEN], *dlinkname_ptr;
4950
ulong pack_reclength;
5051
ulonglong tot_length,max_rows, tmp;
5152
enum en_fieldtype type;
@@ -591,19 +592,19 @@ int mi_create(const char *name,uint keys,MI_KEYDEF *keydefs,
591592
/* chop off the table name, tempory tables use generated name */
592593
if ((path= strrchr(ci->index_file_name, FN_LIBCHAR)))
593594
*path= '\0';
594-
fn_format(filename, name, ci->index_file_name, MI_NAME_IEXT,
595+
fn_format(kfilename, name, ci->index_file_name, MI_NAME_IEXT,
595596
MY_REPLACE_DIR | MY_UNPACK_FILENAME |
596597
MY_RETURN_REAL_PATH | MY_APPEND_EXT);
597598
}
598599
else
599600
{
600-
fn_format(filename, ci->index_file_name, "", MI_NAME_IEXT,
601+
fn_format(kfilename, ci->index_file_name, "", MI_NAME_IEXT,
601602
MY_UNPACK_FILENAME | MY_RETURN_REAL_PATH |
602603
(have_iext ? MY_REPLACE_EXT : MY_APPEND_EXT));
603604
}
604-
fn_format(linkname, name, "", MI_NAME_IEXT,
605+
fn_format(klinkname, name, "", MI_NAME_IEXT,
605606
MY_UNPACK_FILENAME|MY_APPEND_EXT);
606-
linkname_ptr=linkname;
607+
klinkname_ptr= klinkname;
607608
/*
608609
Don't create the table if the link or file exists to ensure that one
609610
doesn't accidently destroy another table.
@@ -614,10 +615,10 @@ int mi_create(const char *name,uint keys,MI_KEYDEF *keydefs,
614615
{
615616
char *iext= strrchr(name, '.');
616617
int have_iext= iext && !strcmp(iext, MI_NAME_IEXT);
617-
fn_format(filename, name, "", MI_NAME_IEXT,
618+
fn_format(kfilename, name, "", MI_NAME_IEXT,
618619
MY_UNPACK_FILENAME | MY_RETURN_REAL_PATH |
619620
(have_iext ? MY_REPLACE_EXT : MY_APPEND_EXT));
620-
linkname_ptr=0;
621+
klinkname_ptr= 0;
621622
/* Replace the current file */
622623
create_flag=(flags & HA_CREATE_KEEP_FILES) ? 0 : MY_DELETE_OLD;
623624
}
@@ -632,7 +633,7 @@ int mi_create(const char *name,uint keys,MI_KEYDEF *keydefs,
632633
NOTE: The filename is compared against unique_file_name of every
633634
open table. Hence we need a real path here.
634635
*/
635-
if (test_if_reopen(filename))
636+
if (test_if_reopen(kfilename))
636637
{
637638
my_printf_error(HA_ERR_TABLE_EXIST, "MyISAM table '%s' is in use "
638639
"(most likely by a MERGE table). Try FLUSH TABLES.",
@@ -642,7 +643,7 @@ int mi_create(const char *name,uint keys,MI_KEYDEF *keydefs,
642643
}
643644

644645
if ((file= mysql_file_create_with_symlink(mi_key_file_kfile,
645-
linkname_ptr, filename, 0,
646+
klinkname_ptr, kfilename, 0,
646647
create_mode,
647648
MYF(MY_WME | create_flag))) < 0)
648649
goto err;
@@ -662,31 +663,31 @@ int mi_create(const char *name,uint keys,MI_KEYDEF *keydefs,
662663
/* chop off the table name, tempory tables use generated name */
663664
if ((path= strrchr(ci->data_file_name, FN_LIBCHAR)))
664665
*path= '\0';
665-
fn_format(filename, name, ci->data_file_name, MI_NAME_DEXT,
666+
fn_format(dfilename, name, ci->data_file_name, MI_NAME_DEXT,
666667
MY_REPLACE_DIR | MY_UNPACK_FILENAME | MY_APPEND_EXT);
667668
}
668669
else
669670
{
670-
fn_format(filename, ci->data_file_name, "", MI_NAME_DEXT,
671+
fn_format(dfilename, ci->data_file_name, "", MI_NAME_DEXT,
671672
MY_UNPACK_FILENAME |
672673
(have_dext ? MY_REPLACE_EXT : MY_APPEND_EXT));
673674
}
674675

675-
fn_format(linkname, name, "",MI_NAME_DEXT,
676+
fn_format(dlinkname, name, "",MI_NAME_DEXT,
676677
MY_UNPACK_FILENAME | MY_APPEND_EXT);
677-
linkname_ptr=linkname;
678+
dlinkname_ptr= dlinkname;
678679
create_flag=0;
679680
}
680681
else
681682
{
682-
fn_format(filename,name,"", MI_NAME_DEXT,
683+
fn_format(dfilename,name,"", MI_NAME_DEXT,
683684
MY_UNPACK_FILENAME | MY_APPEND_EXT);
684-
linkname_ptr=0;
685+
dlinkname_ptr= 0;
685686
create_flag=(flags & HA_CREATE_KEEP_FILES) ? 0 : MY_DELETE_OLD;
686687
}
687688
if ((dfile=
688689
mysql_file_create_with_symlink(mi_key_file_dfile,
689-
linkname_ptr, filename, 0,
690+
dlinkname_ptr, dfilename, 0,
690691
create_mode,
691692
MYF(MY_WME | create_flag))) < 0)
692693
goto err;
@@ -838,19 +839,21 @@ int mi_create(const char *name,uint keys,MI_KEYDEF *keydefs,
838839
(void) mysql_file_close(dfile, MYF(0));
839840
/* fall through */
840841
case 2:
841-
if (! (flags & HA_DONT_TOUCH_DATA))
842-
mysql_file_delete_with_symlink(mi_key_file_dfile,
843-
fn_format(filename, name, "", MI_NAME_DEXT,
844-
MY_UNPACK_FILENAME | MY_APPEND_EXT),
845-
MYF(0));
842+
if (! (flags & HA_DONT_TOUCH_DATA))
843+
{
844+
mysql_file_delete(mi_key_file_dfile, dfilename, MYF(0));
845+
if (dlinkname_ptr)
846+
mysql_file_delete(mi_key_file_dfile, dlinkname_ptr, MYF(0));
847+
}
846848
/* fall through */
847849
case 1:
848850
(void) mysql_file_close(file, MYF(0));
849851
if (! (flags & HA_DONT_TOUCH_DATA))
850-
mysql_file_delete_with_symlink(mi_key_file_kfile,
851-
fn_format(filename, name, "", MI_NAME_IEXT,
852-
MY_UNPACK_FILENAME | MY_APPEND_EXT),
853-
MYF(0));
852+
{
853+
mysql_file_delete(mi_key_file_kfile, kfilename, MYF(0));
854+
if (klinkname_ptr)
855+
mysql_file_delete(mi_key_file_kfile, klinkname_ptr, MYF(0));
856+
}
854857
}
855858
my_free(rec_per_key_part);
856859
DBUG_RETURN(my_errno=save_errno); /* return the fatal errno */

0 commit comments

Comments
 (0)