Fix rig_cookie: use lock for reads and write.

Ensure we never print more then HAMLIB_COOKIE_SIZE otherwise we read
out-of-bounds.
Drop stray printf.
Add tests for invalid input and overly large input.
Fix test2 to release the cookie.

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);
-        return -RIG_EINVAL; // wrong cookie
+            ret = -RIG_EINVAL; // wrong cookie
+        }
 
         break;
 
@@ -6864,25 +6872,17 @@ 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;
+        double time_curr = tp.tv_sec + tp.tv_nsec / 1e9;
-
-#ifdef HAVE_PTHREAD
-        pthread_mutex_lock(&cookie_lock);
-#endif
 
         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
+            ret = -RIG_BUSBUSY;
-            pthread_mutex_unlock(&cookie_lock);
-#endif
-            return -RIG_BUSBUSY;
         }
-
+        else
-
+        {
             if (cookie_save[0] != 0)
             {
                 rig_debug(RIG_DEBUG_ERR,
@@ -6890,22 +6890,30 @@ int HAMLIB_API rig_cookie(RIG *rig, enum cookie_e cookie_cmd, char *cookie,
                       __FILE__, __LINE__, cookie_save, time_curr - time_last_used);
             }
 
-        date_strget(cookie_save, sizeof(cookie_save));
+            date_strget(cookie, cookie_len);
+            size_t len = strlen(cookie);
             // add on our random number to ensure uniqueness
-        snprintf(cookie, cookie_len, "%s %d\n", cookie_save, rand());
+            // 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);
-#ifdef HAVE_PTHREAD
+            ret = RIG_OK;
-        pthread_mutex_unlock(&cookie_lock);
+        }
-#endif
+        break;
-        return RIG_OK;
+
+    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__);
+#ifdef HAVE_PTHREAD
-    return -RIG_EPROTO;
+    pthread_mutex_unlock(&cookie_lock);
+#endif
+    return ret;
+
 }
 
 HAMLIB_EXPORT(void) sync_callback(int lock)

tests/testcookie.c

@@ -1,7 +1,7 @@
 #include <hamlib/rig.h>
 
 // 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;
 }