From c285985eeb25a08124fec44c5872a2d9878470ee Mon Sep 17 00:00:00 2001 From: Cedric BAIL Date: Fri, 5 Mar 2010 17:19:03 +0000 Subject: [PATCH] * eet: Fix clearcache race condition. Patch by Adam Simpkins. SVN revision: 46891 --- legacy/eet/AUTHORS | 1 + legacy/eet/ChangeLog | 4 ++ legacy/eet/src/lib/eet_lib.c | 63 ++++++++++++++++++++----------- legacy/eet/src/tests/eet_suite.c | 64 ++++++++++++++++++++++++++++++++ 4 files changed, 111 insertions(+), 21 deletions(-) diff --git a/legacy/eet/AUTHORS b/legacy/eet/AUTHORS index cd5fc41184..d1172a0e26 100644 --- a/legacy/eet/AUTHORS +++ b/legacy/eet/AUTHORS @@ -9,3 +9,4 @@ Gustavo Sverzut Barbieri Raphael Kubo da Costa Mathieu Taillefumier Albin "Lutin" Tonnerre +Adam Simpkins diff --git a/legacy/eet/ChangeLog b/legacy/eet/ChangeLog index f71ff865c8..093d030808 100644 --- a/legacy/eet/ChangeLog +++ b/legacy/eet/ChangeLog @@ -337,3 +337,7 @@ 2010-03-01 Albin Tonnerre * Fix override of global symbols. + +2010-03-05 Adam Simpkins + + * Fix clearcache race condition. diff --git a/legacy/eet/src/lib/eet_lib.c b/legacy/eet/src/lib/eet_lib.c index 8fed189666..4ed8432f60 100644 --- a/legacy/eet/src/lib/eet_lib.c +++ b/legacy/eet/src/lib/eet_lib.c @@ -280,7 +280,7 @@ eet_test_close(int test, Eet_File *ef) if (test) { ef->delete_me_now = 1; - eet_internal_close(ef, 0); + eet_internal_close(ef, 1); } return test; } @@ -307,6 +307,7 @@ eet_cache_find(const char *path, Eet_File **cache, int cache_num) } /* add to end of cache */ +/* this should only be called when the cache lock is already held */ static void eet_cache_add(Eet_File *ef, Eet_File ***cache, int *cache_num, int *cache_alloc) { @@ -358,6 +359,7 @@ eet_cache_add(Eet_File *ef, Eet_File ***cache, int *cache_num, int *cache_alloc) } /* delete from cache */ +/* this should only be called when the cache lock is already held */ static void eet_cache_del(Eet_File *ef, Eet_File ***cache, int *cache_num, int *cache_alloc) { @@ -849,6 +851,7 @@ eet_clearcache(void) * We need to compute the list of eet file to close separately from the cache, * due to eet_close removing them from the cache after each call. */ + LOCK_CACHE; for (i = 0; i < eet_writers_num; i++) { if (eet_writers[i]->references <= 0) num++; @@ -887,9 +890,10 @@ eet_clearcache(void) for (i = 0; i < num; i++) { - eet_close(closelist[i]); + eet_internal_close(closelist[i], 1); } } + UNLOCK_CACHE; } /* FIXME: MMAP race condition in READ_WRITE_MODE */ @@ -1281,6 +1285,12 @@ eet_internal_read1(Eet_File *ef) } #endif +/* + * this should only be called when the cache lock is already held + * (We could drop this restriction if we add a parameter to eet_test_close + * that indicates if the lock is held or not. For now it is easiest + * to just require that it is always held.) + */ static Eet_File * eet_internal_read(Eet_File *ef) { @@ -1317,6 +1327,9 @@ eet_internal_close(Eet_File *ef, Eina_Bool locked) /* check to see its' an eet file pointer */ if (eet_check_pointer(ef)) return EET_ERROR_BAD_OBJECT; + + if (!locked) LOCK_CACHE; + /* deref */ ef->references--; /* if its still referenced - dont go any further */ @@ -1329,17 +1342,18 @@ eet_internal_close(Eet_File *ef, Eina_Bool locked) /* if not urgent to delete it - dont free it - leave it in cache */ if ((!ef->delete_me_now) && (ef->mode == EET_FILE_MODE_READ)) - return EET_ERROR_NONE; + { + if (!locked) UNLOCK_CACHE; + return EET_ERROR_NONE; + } /* remove from cache */ - if (!locked) - { - LOCK_CACHE; - } if (ef->mode == EET_FILE_MODE_READ) eet_cache_del(ef, &eet_readers, &eet_readers_num, &eet_readers_alloc); else if ((ef->mode == EET_FILE_MODE_WRITE) || (ef->mode == EET_FILE_MODE_READ_WRITE)) eet_cache_del(ef, &eet_writers, &eet_writers_num, &eet_writers_alloc); + + /* we can unlock the cache now */ if (!locked) { UNLOCK_CACHE; @@ -1425,7 +1439,11 @@ eet_memopen_read(const void *data, size_t size) ef->sha1 = NULL; ef->sha1_length = 0; - return eet_internal_read(ef); + /* eet_internal_read expects the cache lock to be held when it is called */ + LOCK_CACHE; + ef = eet_internal_read(ef); + UNLOCK_CACHE; + return ef; } EAPI Eet_File * @@ -1466,7 +1484,6 @@ eet_open(const char *file, Eet_File_Mode mode) } ef = eet_cache_find((char *)file, eet_writers, eet_writers_num); } - UNLOCK_CACHE; /* try open the file based on mode */ if ((mode == EET_FILE_MODE_READ) || (mode == EET_FILE_MODE_READ_WRITE)) @@ -1475,23 +1492,23 @@ eet_open(const char *file, Eet_File_Mode mode) file_stat.st_mtime = 0; fp = fopen(file, "rb"); - if (!fp) goto on_error; + if (!fp) goto open_error; if (fstat(fileno(fp), &file_stat)) { fclose(fp); fp = NULL; - goto on_error; + goto open_error; } if ((mode == EET_FILE_MODE_READ) && (file_stat.st_size < ((int) sizeof(int) * 3))) { fclose(fp); fp = NULL; - goto on_error; + goto open_error; } - on_error: - if (fp == NULL && mode == EET_FILE_MODE_READ) return NULL; + open_error: + if (fp == NULL && mode == EET_FILE_MODE_READ) goto on_error; } else { @@ -1503,7 +1520,7 @@ eet_open(const char *file, Eet_File_Mode mode) unlink(file); fd = open(file, O_CREAT | O_TRUNC | O_RDWR, S_IRUSR | S_IWUSR); fp = fdopen(fd, "wb"); - if (!fp) return NULL; + if (!fp) goto on_error; } /* We found one */ @@ -1520,6 +1537,7 @@ eet_open(const char *file, Eet_File_Mode mode) /* reference it up and return it */ if (fp != NULL) fclose(fp); ef->references++; + UNLOCK_CACHE; return ef; } @@ -1528,7 +1546,7 @@ eet_open(const char *file, Eet_File_Mode mode) /* Allocate struct for eet file and have it zero'd out */ ef = malloc(sizeof(Eet_File) + file_len); if (!ef) - return NULL; + goto on_error; /* fill some of the members */ INIT_FILE(ef); @@ -1557,7 +1575,7 @@ eet_open(const char *file, Eet_File_Mode mode) /* if we can't open - bail out */ if (eet_test_close(!ef->fp, ef)) - return NULL; + goto on_error; fcntl(fileno(ef->fp), F_SETFD, FD_CLOEXEC); /* if we opened for read or read-write */ @@ -1567,10 +1585,10 @@ eet_open(const char *file, Eet_File_Mode mode) ef->data = mmap(NULL, ef->data_size, PROT_READ, MAP_SHARED, fileno(ef->fp), 0); if (eet_test_close((ef->data == MAP_FAILED), ef)) - return NULL; + goto on_error; ef = eet_internal_read(ef); if (!ef) - return NULL; + goto on_error; } empty_file: @@ -1584,16 +1602,19 @@ eet_open(const char *file, Eet_File_Mode mode) /* add to cache */ if (ef->references == 1) { - LOCK_CACHE; if (ef->mode == EET_FILE_MODE_READ) eet_cache_add(ef, &eet_readers, &eet_readers_num, &eet_readers_alloc); else if ((ef->mode == EET_FILE_MODE_WRITE) || (ef->mode == EET_FILE_MODE_READ_WRITE)) eet_cache_add(ef, &eet_writers, &eet_writers_num, &eet_writers_alloc); - UNLOCK_CACHE; } + UNLOCK_CACHE; return ef; + +on_error: + UNLOCK_CACHE; + return NULL; } EAPI Eet_File_Mode diff --git a/legacy/eet/src/tests/eet_suite.c b/legacy/eet/src/tests/eet_suite.c index a41e8f7d23..0faa01b883 100644 --- a/legacy/eet/src/tests/eet_suite.c +++ b/legacy/eet/src/tests/eet_suite.c @@ -6,6 +6,7 @@ #include #include #include +#include #include @@ -1410,6 +1411,65 @@ START_TEST(eet_cipher_decipher_simple) } END_TEST +static Eina_Bool open_worker_stop; +static void* +open_close_worker(void* path) +{ + while (!open_worker_stop) + { + Eet_File* ef = eet_open((char const*)path, EET_FILE_MODE_READ); + if (ef == NULL) + { + pthread_exit("eet_open() failed"); + } + else + { + Eet_Error err_code = eet_close(ef); + if (err_code != EET_ERROR_NONE) + pthread_exit("eet_close() failed"); + } + } + + pthread_exit(NULL); +} + +START_TEST(eet_cache_concurrency) +{ + char *file = strdup("/tmp/eet_suite_testXXXXXX"); + const char *buffer = "test data"; + Eet_File *ef; + void *thread_ret; + unsigned int n; + + eet_init(); + + /* create a file to test with */ + fail_if(!mktemp(file)); + ef = eet_open(file, EET_FILE_MODE_WRITE); + fail_if(!ef); + fail_if(!eet_write(ef, "keys/tests", buffer, strlen(buffer) + 1, 0)); + + /* start a thread that repeatedly opens and closes a file */ + open_worker_stop = 0; + pthread_t thread; + pthread_create(&thread, NULL, open_close_worker, file); + + /* clear the cache repeatedly in this thread */ + for (n = 0; n < 100000; ++n) + { + eet_clearcache(); + } + + /* join the other thread, and fail if it returned an error message */ + open_worker_stop = 1; + fail_if(pthread_join(thread, &thread_ret) != 0); + fail_unless(thread_ret == NULL, (char const*)thread_ret); + + fail_if(unlink(file) != 0); + eet_shutdown(); +} +END_TEST + Suite * eet_suite(void) @@ -1455,6 +1515,10 @@ eet_suite(void) suite_add_tcase(s, tc); #endif + tc = tcase_create("Eet Cache"); + tcase_add_test(tc, eet_cache_concurrency); + suite_add_tcase(s, tc); + return s; }