diff --git a/src/rig.c b/src/rig.c index 3ef581536..5c4ab9aa4 100644 --- a/src/rig.c +++ b/src/rig.c @@ -6791,16 +6791,16 @@ int HAMLIB_API rig_cookie(RIG *rig, enum cookie_e cookie_cmd, char *cookie, // only 1 client can have the cookie so these can be static // this should also prevent problems with DLLs & shared libraies // the debug_msg is another non-thread-safe which this will help fix - // 27 char cookie will last until the year 10000 static char - cookie_save[HAMLIB_COOKIE_SIZE]; // only one client can have the 26-char cookie + cookie_save[HAMLIB_COOKIE_SIZE]; // only one client can have the cookie static double time_last_used; - double time_curr; struct timespec tp; + int ret; #ifdef HAVE_PTHREAD static pthread_mutex_t cookie_lock = PTHREAD_MUTEX_INITIALIZER; #endif + /* This is not needed for RIG_COOKIE_RELEASE but keep it simple. */ if (cookie_len < HAMLIB_COOKIE_SIZE) { rig_debug(RIG_DEBUG_ERR, "%s(%d): cookie_len < %d\n", @@ -6815,6 +6815,12 @@ int HAMLIB_API rig_cookie(RIG *rig, enum cookie_e cookie_cmd, char *cookie, return -RIG_EINVAL; // nothing to do } + /* Accesing cookie_save and time_last_used must be done with lock held. + * So keep code simple and lock it during the whole operation. */ +#ifdef HAVE_PTHREAD + pthread_mutex_lock(&cookie_lock); +#endif + switch (cookie_cmd) { case RIG_COOKIE_RELEASE: @@ -6824,14 +6830,14 @@ int HAMLIB_API rig_cookie(RIG *rig, enum cookie_e cookie_cmd, char *cookie, rig_debug(RIG_DEBUG_VERBOSE, "%s(%d): %s cookie released\n", __FILE__, __LINE__, cookie_save); memset(cookie_save, 0, sizeof(cookie_save)); - return RIG_OK; + ret = RIG_OK; } else // not the right cookie!! { rig_debug(RIG_DEBUG_ERR, "%s(%d): %s can't release cookie as cookie %s is active\n", __FILE__, __LINE__, cookie, cookie_save); - return -RIG_BUSBUSY; + ret = -RIG_BUSBUSY; } break; @@ -6847,13 +6853,15 @@ int HAMLIB_API rig_cookie(RIG *rig, enum cookie_e cookie_cmd, char *cookie, __LINE__, cookie); clock_gettime(CLOCK_REALTIME, &tp); time_last_used = tp.tv_sec + tp.tv_nsec / 1e9; - return RIG_OK; + ret = RIG_OK; + } + else + { + rig_debug(RIG_DEBUG_ERR, + "%s(%d): %s renew request refused %s is active\n", + __FILE__, __LINE__, cookie, cookie_save); + ret = -RIG_EINVAL; // wrong cookie } - - rig_debug(RIG_DEBUG_ERR, - "%s(%d): %s renew request refused %s is active\n", - __FILE__, __LINE__, cookie, cookie_save); - return -RIG_EINVAL; // wrong cookie break; @@ -6864,48 +6872,48 @@ int HAMLIB_API rig_cookie(RIG *rig, enum cookie_e cookie_cmd, char *cookie, // we are just allow for a crashed client that fails to release:q clock_gettime(CLOCK_REALTIME, &tp); - time_curr = tp.tv_sec + tp.tv_nsec / 1e9; - -#ifdef HAVE_PTHREAD - pthread_mutex_lock(&cookie_lock); -#endif + double time_curr = tp.tv_sec + tp.tv_nsec / 1e9; if (cookie_save[0] != 0 && (strcmp(cookie_save, cookie) == 0) && (time_curr - time_last_used < 1)) // then we will deny the request { - printf("Cookie %s in use\n", cookie_save); rig_debug(RIG_DEBUG_ERR, "%s(%d): %s cookie is in use\n", __FILE__, __LINE__, cookie_save); -#ifdef HAVE_PTHREAD - pthread_mutex_unlock(&cookie_lock); -#endif - return -RIG_BUSBUSY; + ret = -RIG_BUSBUSY; } - - - if (cookie_save[0] != 0) + else { - rig_debug(RIG_DEBUG_ERR, + if (cookie_save[0] != 0) + { + rig_debug(RIG_DEBUG_ERR, "%s(%d): %s cookie has expired after %.3f seconds....overriding with new cookie\n", __FILE__, __LINE__, cookie_save, time_curr - time_last_used); - } + } - date_strget(cookie_save, sizeof(cookie_save)); - // add on our random number to ensure uniqueness - snprintf(cookie, cookie_len, "%s %d\n", cookie_save, rand()); - strcpy(cookie_save, cookie); - time_last_used = time_curr; - rig_debug(RIG_DEBUG_VERBOSE, "%s(%d): %s new cookie request granted\n", - __FILE__, __LINE__, cookie_save); -#ifdef HAVE_PTHREAD - pthread_mutex_unlock(&cookie_lock); -#endif - return RIG_OK; + date_strget(cookie, cookie_len); + size_t len = strlen(cookie); + // add on our random number to ensure uniqueness + // The cookie should never be longer then HAMLIB_COOKIE_SIZE + snprintf(cookie + len, HAMLIB_COOKIE_SIZE - len, " %d\n", rand()); + strcpy(cookie_save, cookie); + time_last_used = time_curr; + rig_debug(RIG_DEBUG_VERBOSE, "%s(%d): %s new cookie request granted\n", + __FILE__, __LINE__, cookie_save); + ret = RIG_OK; + } + break; + + default: + rig_debug(RIG_DEBUG_ERR, "%s(%d): unknown cmd!!\n'", __FILE__, __LINE__); + ret = -RIG_EPROTO; break; } - rig_debug(RIG_DEBUG_ERR, "%s(%d): unknown cmd!!\n'", __FILE__, __LINE__); - return -RIG_EPROTO; +#ifdef HAVE_PTHREAD + pthread_mutex_unlock(&cookie_lock); +#endif + return ret; + } HAMLIB_EXPORT(void) sync_callback(int lock) diff --git a/tests/testcookie.c b/tests/testcookie.c index bdb22e7b1..94c5fa653 100644 --- a/tests/testcookie.c +++ b/tests/testcookie.c @@ -1,7 +1,7 @@ #include // GET tests -int test1() +static int test1() { int retcode; // Normal get @@ -45,7 +45,7 @@ int test1() } // RENEW tests -int test2() +static int test2() { int retcode; char cookie[HAMLIB_COOKIE_SIZE]; @@ -72,9 +72,61 @@ int test2() if (retcode != RIG_OK) { printf("Test#2d OK\n"); } else {printf("Test#2d Failed cookie=%s\n", cookie); return 1;} +// release cookie2 again to clean up test + retcode = rig_cookie(NULL, RIG_COOKIE_RELEASE, cookie2, sizeof(cookie2)); + + if (retcode == RIG_OK) { printf("Test#2e OK\n"); } + else {printf("Test#2e Failed\n"); return 1;} return 0; } +// Input sanity checks +static int test3_invalid_input() +{ + int retcode; + char cookie[HAMLIB_COOKIE_SIZE]; + /* Make sure any value smaller then HAMLIB_COOKIE_SIZE is rejected */ + for(unsigned int i = 0; i < HAMLIB_COOKIE_SIZE; i++) + { + retcode = rig_cookie(NULL, RIG_COOKIE_GET, cookie, i); + if (retcode == -RIG_EINVAL) { printf("Test#3a OK\n"); } + else {printf("Test#3a Failed\n"); return 1;} + } + + /* Make sure a NULL cookie is ignored */ + retcode = rig_cookie(NULL, RIG_COOKIE_GET, NULL, sizeof(cookie)); + if (retcode == -RIG_EINVAL) { printf("Test#3b OK\n"); } + else {printf("Test#3b Failed\n"); return 1;} + + /* Make sure an invalid command is dropped with proto error */ + retcode = rig_cookie(NULL, RIG_COOKIE_RENEW + 1, cookie, sizeof(cookie)); + if (retcode == -RIG_EPROTO) { printf("Test#3c OK\n"); } + else {printf("Test#3c Failed\n"); return 1;} + + return 0; +} + +static int test4_large_cookie_size() +{ + int retcode; + char cookie[HAMLIB_COOKIE_SIZE * 2]; + + /* Using a larger cookie should also work */ + retcode = rig_cookie(NULL, RIG_COOKIE_GET, cookie, sizeof(cookie)); + if (retcode == RIG_OK) { printf("Test#4a OK\n"); } + else {printf("Test#4a Failed\n"); return 1;} + + /* Cookie should be smaller the maximum specified by lib */ + if (strlen(cookie) < HAMLIB_COOKIE_SIZE) { printf("Test#4b OK\n"); } + else {printf("Test#4b Failed\n"); return 1;} + + /* Release the cookie again to clean up */ + retcode = rig_cookie(NULL, RIG_COOKIE_RELEASE, cookie, sizeof(cookie)); + if (retcode == RIG_OK) { printf("Test#4c OK\n"); } + else {printf("Test#4c Failed\n"); return 1;} + + return 0; +} int main() { @@ -84,5 +136,9 @@ int main() if (test2()) { return 1; } + if (test3_invalid_input()) { return 1; } + + if (test4_large_cookie_size()) { return 1; } + return 0; }