-
Notifications
You must be signed in to change notification settings - Fork 0
WIP: Prevent race condition around strerror which is not thread safe #3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: unstable
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| /* | ||
| * strerror is not thread safe | ||
| * why oh why should I complain | ||
| * when strerror_r & _l behave differently | ||
| * Across the Unix domain | ||
| * | ||
| * after distinguishing between the two | ||
| * And allocating error strings all over | ||
| * You have ugly Marcos | ||
| * (and are still not async signal safe) | ||
| * | ||
| * Allocate a static array containing all possible error strings | ||
| */ | ||
|
|
||
| #include <errno.h> | ||
| #include <string.h> | ||
| #include <stdio.h> | ||
|
|
||
| #include "redis_err.h" | ||
| #include "zmalloc.h" | ||
|
|
||
| const char **ERROR_ARRAY; | ||
| /* sys_nerr is considered internal impl. detail, and isn't available on all OSes */ | ||
| const unsigned int MAX_ERR = 256; | ||
| const char *UNKNOWN_STRING = "Unknown error"; | ||
|
|
||
| /* | ||
| * Initialize the error array | ||
| */ | ||
| int initErrorStrings(){ | ||
| ERROR_ARRAY = zmalloc(sizeof(char*)*(MAX_ERR)); | ||
|
|
||
| unsigned int n = 0; | ||
| char *err; | ||
|
|
||
| for (; n < MAX_ERR; n++) { | ||
| err = strerror(n); | ||
| if (err == NULL | ||
| || strncasecmp(err, UNKNOWN_STRING, 13) == 0) | ||
| { | ||
| ERROR_ARRAY[n] = UNKNOWN_STRING; | ||
| continue; | ||
| } | ||
| ERROR_ARRAY[n] = zstrdup(err); | ||
| } | ||
| return 0; | ||
| } | ||
|
|
||
|
|
||
| const char *redisError(unsigned int err) { | ||
| if (err > MAX_ERR) { | ||
| perror("Requested out of bounds error string"); | ||
| return UNKNOWN_STRING; | ||
| } | ||
| return ERROR_ARRAY[err]; | ||
| } | ||
|
|
||
|
|
||
| void freeErrorStrings() { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. redis doesn't bother to free memory.. you can keep this method, but don't bother to call it. |
||
| unsigned int n = 0; | ||
| for (;n < MAX_ERR; n++) { | ||
| if (ERROR_ARRAY[n] != UNKNOWN_STRING) { | ||
| zfree((void *)ERROR_ARRAY[n]); | ||
| } | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| // | ||
| // Created by uri on 6/7/19. | ||
| // | ||
|
|
||
| #ifndef REDIS_REDIS_ERR_H | ||
| #define REDIS_REDIS_ERR_H | ||
|
|
||
|
|
||
| int initErrorStrings (); | ||
| const char *redisError(unsigned int err); | ||
| void freeErrorStrings(); | ||
|
|
||
| #endif //REDIS_REDIS_ERR_H |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,7 @@ | |
| #include "bio.h" | ||
| #include "latency.h" | ||
| #include "atomicvar.h" | ||
| #include "redis_err.h" | ||
|
|
||
| #include <time.h> | ||
| #include <signal.h> | ||
|
|
@@ -1767,7 +1768,7 @@ void checkChildrenDone(void) { | |
| if (pid == -1) { | ||
| serverLog(LL_WARNING,"wait3() returned an error: %s. " | ||
| "rdb_child_pid = %d, aof_child_pid = %d, module_child_pid = %d", | ||
| strerror(errno), | ||
| redisError(errno), | ||
| (int) server.rdb_child_pid, | ||
| (int) server.aof_child_pid, | ||
| (int) server.module_child_pid); | ||
|
|
@@ -2843,7 +2844,7 @@ void initServer(void) { | |
| O_WRONLY|O_APPEND|O_CREAT,0644); | ||
| if (server.aof_fd == -1) { | ||
| serverLog(LL_WARNING, "Can't open the append-only file: %s", | ||
| strerror(errno)); | ||
| redisError(errno)); | ||
| exit(1); | ||
| } | ||
| } | ||
|
|
@@ -4773,7 +4774,7 @@ void loadDataFromDisk(void) { | |
| selectDb(server.cached_master,rsi.repl_stream_db); | ||
| } | ||
| } else if (errno != ENOENT) { | ||
| serverLog(LL_WARNING,"Fatal error loading the DB: %s. Exiting.",strerror(errno)); | ||
| serverLog(LL_WARNING,"Fatal error loading the DB: %s. Exiting.",redisError(errno)); | ||
| exit(1); | ||
| } | ||
| } | ||
|
|
@@ -4902,6 +4903,7 @@ int main(int argc, char **argv) { | |
| setlocale(LC_COLLATE,""); | ||
| tzset(); /* Populates 'timezone' global. */ | ||
| zmalloc_set_oom_handler(redisOutOfMemoryHandler); | ||
| initErrorStrings(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the unit test code above may some day use this mechanism too (by calling a method that calls this). unlikely, but maybe you should consider moving this to the top. |
||
| srand(time(NULL)^getpid()); | ||
| gettimeofday(&tv,NULL); | ||
|
|
||
|
|
@@ -5073,6 +5075,7 @@ int main(int argc, char **argv) { | |
| aeSetAfterSleepProc(server.el,afterSleep); | ||
| aeMain(server.el); | ||
| aeDeleteEventLoop(server.el); | ||
| freeErrorStrings(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is actually dead code.. the normal place where redis exists, it with a call to |
||
| return 0; | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think you should print anything. no one usually monitors stderr, and not sure there's a point in printing anyway since the returned string probably goes to the log.
what can be helpful is stack trace, in case the one that prints to the log doesn't have a distinct message.