Reorder some operations to minimize a race condition

sched/mqueue/mq_send.c

@@ -172,7 +172,13 @@ int mq_send(mqd_t mqdes, FAR const char *msg, size_t msglen, int prio)
 
   if (mqmsg)
     {
-      /* Yes, perform the message send. */
+      /* Yes, perform the message send.
+       *
+       * NOTE: There is a race condition here: What if a message is added by
+       * interrupt related logic so that queue again becomes non-empty.
+       * That is handled because mq_dosend() will permit the maxmsgs limit
+       * to be exceeded in that case.
+       */
 
       ret = mq_dosend(mqdes, mqmsg, msg, msglen, prio);
     }

sched/mqueue/mq_timedsend.c

@@ -202,8 +202,20 @@ int mq_timedsend(mqd_t mqdes, FAR const char *msg, size_t msglen, int prio,
       return ERROR;
     }
 
+  /* Pre-allocate a message structure */
+
+  mqmsg = mq_msgalloc();
+  if (!mqmsg)
+    {
+      /* Failed to allocate the message */
+
+      set_errno(ENOMEM);
+      return ERROR;
+    }
+
   /* Get a pointer to the message queue */
 
+  sched_lock();
   msgq = mqdes->msgq;
 
   /* OpenGroup.org: "Under no circumstance shall the operation fail with a
@@ -213,16 +225,18 @@ int mq_timedsend(mqd_t mqdes, FAR const char *msg, size_t msglen, int prio,
    *
    * Also ignore the time value if for some crazy reason we were called from
    * an interrupt handler.  This probably really should be an assertion.
+   *
+   * NOTE: There is a race condition here: What if a message is added by
+   * interrupt related logic so that queue again becomes non-empty.  That
+   * is handled because mq_dosend() will permit the maxmsgs limit to be
+   * exceeded in that case.
    */
 
-  sched_lock();
   if (msgq->nmsgs < msgq->maxmsgs || up_interrupt_context())
     {
-      /* The message queue is not full... Ignore the time parameter and
+      /* Do the send with no further checks (possibly exceeding maxmsgs) */
-       * let mq_send do the work.
-       */
 
-      ret = mq_send(mqdes, msg, msglen, prio);
+      ret = mq_dosend(mqdes, mqmsg, msg, msglen, prio);
       sched_unlock();
       return ret;
     }
@@ -233,9 +247,8 @@ int mq_timedsend(mqd_t mqdes, FAR const char *msg, size_t msglen, int prio,
 
   if (!abstime || abstime->tv_nsec < 0 || abstime->tv_nsec >= 1000000000)
     {
-      set_errno(EINVAL);
+      result = EINVAL;
-      sched_unlock();
+      goto errout_with_mqmsg;
-      return ERROR;
     }
 
   /* Create a watchdog.  We will not actually need this watchdog
@@ -246,9 +259,8 @@ int mq_timedsend(mqd_t mqdes, FAR const char *msg, size_t msglen, int prio,
   rtcb->waitdog = wd_create();
   if (!rtcb->waitdog)
     {
-      set_errno(EINVAL);
+      result = EINVAL;
-      sched_unlock();
+      goto errout_with_mqmsg;
-      return ERROR;
     }
 
   /* We are not in an interrupt handler and the message queue is full.
@@ -268,33 +280,35 @@ int mq_timedsend(mqd_t mqdes, FAR const char *msg, size_t msglen, int prio,
   if (result == OK && ticks <= 0)
     {
       result = ETIMEDOUT;
+      goto errout_with_irqsave;
     }
 
   /* Handle any time-related errors */
 
   if (result != OK)
     {
-      set_errno(result);
+      goto errout_with_irqsave;
-      ret = ERROR;
     }
 
   /* Start the watchdog and begin the wait for MQ not full */
 
-  else
+  wd_start(rtcb->waitdog, ticks, (wdentry_t)mq_sndtimeout, 1, getpid());
+
+  /* And wait for the message queue to be non-empty */
+
+  ret = mq_waitsend(mqdes);
+
+  /* This may return with an error and errno set to either EINTR
+   * or ETIMEOUT.  Cancel the watchdog timer in any event.
+   */
+
+  wd_cancel(rtcb->waitdog);
+
+  /* Check if the wait failed */
+
+  if (ret < 0)
     {
-      /* Start the watchdog */
+      goto errout_with_irqsave;
-
-      wd_start(rtcb->waitdog, ticks, (wdentry_t)mq_sndtimeout, 1, getpid());
-
-      /* And wait for the message queue to be non-empty */
-
-      ret = mq_waitsend(mqdes);
-
-      /* This may return with an error and errno set to either EINTR
-       * or ETIMEOUT.  Cancel the watchdog timer in any event.
-       */
-
-      wd_cancel(rtcb->waitdog);
     }
 
   /* That is the end of the atomic operations */
@@ -306,26 +320,30 @@ int mq_timedsend(mqd_t mqdes, FAR const char *msg, size_t msglen, int prio,
    * the message structure.
    */
 
-  if (ret == OK)
+  ret = mq_dosend(mqdes, mqmsg, msg, msglen, prio);
-    {
-      mqmsg = mq_msgalloc();
-      if (!mqmsg)
-        {
-          /* Failed to allocate the message */
-
-          set_errno(ENOMEM);
-          ret = ERROR;
-        }
-      else
-        {
-          /* Perform the message send. */
-
-          ret = mq_dosend(mqdes, mqmsg, msg, msglen, prio);
-        }
-    }
 
   sched_unlock();
   wd_delete(rtcb->waitdog);
   rtcb->waitdog = NULL;
   return ret;
+
+/* Exit here with (1) the scheduler locked, (2) a message allocated, (3) a
+ * wdog allocated, and (4) interrupts disabled.  The error code is in
+ * 'result'
+ */
+
+errout_with_irqsave:
+  irqrestore(saved_state);
+  wd_delete(rtcb->waitdog);
+  rtcb->waitdog = NULL;
+
+/* Exit here with (1) the scheduler locked and 2) a message allocated.  The
+ * error code is in 'result'
+ */
+
+errout_with_mqmsg:
+  mq_msgfree(mqmsg);
+  set_errno(result);
+  sched_unlock();
+  return ERROR;
 }