Useful test coverage; UsefulOutBuf_Compare fixes

inc/qcbor/UsefulBuf.h

@@ -1452,9 +1452,9 @@ UsefulOutBuf_OutUBufOffset(UsefulOutBuf *pUOutBuf, size_t uOffset);
  * substring. @c NULLUsefulBufC is returned if the requested substring
  * is off the end of the output bytes or if in error state.
  */
-UsefulBufC UsefulOutBuf_SubString(UsefulOutBuf *pUOutBuf,
-                                  const size_t  uStart,
-                                  const size_t  uLen);
+UsefulBufC UsefulOutBuf_OutSubString(UsefulOutBuf *pUOutBuf,
+                                     const size_t  uStart,
+                                     const size_t  uLen);
 
 
 /**
@@ -1483,26 +1483,28 @@ static UsefulBuf UsefulOutBuf_RetrieveOutputStorage(UsefulOutBuf *pUOutBuf);
  * @return  0 for equality, positive if uStart1 is lexographically larger,
  *          negative if uStart2 is lexographically larger.
  *
- * This looks into bytes that have been output at the offsets @c start1
- * and @c start2. It compares bytes at those two starting points until
- * they are not equal or @c uLen1 or @c uLen2 is reached. If the
- * length of the string given is off the end of the output data, the
- * string will be effectively truncated to the data in the output
- * buffer for the comparison.
- *
- * This returns positive when @c uStart1 lexographically sorts ahead
- * of @c uStart2 and vice versa.  Zero is returned if the strings
- * compare equally.
- *
- * If lengths are unequal and the first bytes are an exact subset of
- * the second string, then a positve value will be returned and vice
- * versa.
- *
- * If either start is past the end of data in the output buffer, 0
- * will be returned. It is the caller's responsibility to make sure
- * the offsets are not off the end so that a comparison is actually
- * being made. No data will ever be read off the end of the buffer so
- * this safe no matter what offsets are passed.
+ * This looks into bytes that have been output at the offsets
+ * @c start1 and @c start2. It compares bytes at those two starting
+ * points until they are not equal or @c uLen1 or @c uLen2 is
+ * reached. If the length of the string given is off the end of the
+ * output data, the string will be effectively (not actually)
+ * truncated to the data in the output buffer for the comparison.
+ *
+ * This function returns a positive value if the first string
+ * lexicographically precedes the second and a negative value if the
+ * second precedes the first. If the strings are equal, it returns
+ * zero.
+ *
+ * If one string is a substring of the other, the shorter string is
+ * considered smaller. For example, if the first string is "ab" and
+ * the second is "abc", a positive value is returned. The empty string
+ * is always the smallest and sorts ahead of all others.
+ *
+ * If either starting offset is beyond the end of the output buffer,
+ * the function returns zero.  It is the caller’s responsibility to
+ * ensure that offsets are within bounds so that a valid comparison is
+ * performed. No data will ever be read beyond the buffer’s end,
+ * making the function safe regardless of the provided offsets.
  *
  * This is a relatively odd function in that it works on data in the
  * output buffer. It is employed by QCBOR to sort CBOR-encoded maps
@@ -2477,6 +2479,8 @@ static inline size_t UsefulInputBuf_BytesUnconsumed(UsefulInputBuf *pMe)
     * or was never initialized.
     */
    if(pMe->magic != UIB_MAGIC) {
+      /* Strangely gcov thinks the line is not covered by tests even
+       * though it clearly is (tested with break point and printf) */
       return 0;
    }
 
@@ -2711,6 +2715,12 @@ static inline UsefulBufC UsefulInputBuf_RetrieveUndecodedInput(UsefulInputBuf *p
 }
 
 
+/* For backwards compatibility */
+static inline UsefulBufC UsefulOutBuf_SubString(UsefulOutBuf *pMe, const size_t uStart, const size_t uLen)
+{
+   return UsefulOutBuf_OutSubString(pMe, uStart, uLen);
+}
+
 #ifdef __cplusplus
 }
 #endif

src/UsefulBuf.c

@@ -246,11 +246,6 @@ void UsefulOutBuf_Init(UsefulOutBuf *pMe, UsefulBuf Storage)
  */
 void UsefulOutBuf_InsertUsefulBuf(UsefulOutBuf *pMe, UsefulBufC NewData, size_t uInsertionPos)
 {
-   if(pMe->err) {
-      /* Already in error state. */
-      return;
-   }
-
    /* 0. Sanity check the UsefulOutBuf structure
     * A "counter measure". If magic number is not the right number it
     * probably means pMe was not initialized or it was corrupted. Attackers
@@ -262,6 +257,11 @@ void UsefulOutBuf_InsertUsefulBuf(UsefulOutBuf *pMe, UsefulBufC NewData, size_t
       return;  /* Magic number is wrong due to uninitalization or corrption */
    }
 
+   if(pMe->err) {
+      /* Already in error state. */
+      return;
+   }
+
    /* Make sure valid data is less than buffer size. This would only occur
     * if there was corruption of me, but it is also part of the checks to
     * be sure there is no pointer arithmatic under/overflow.
@@ -358,11 +358,6 @@ void UsefulOutBuf_Advance(UsefulOutBuf *pMe, size_t uAmount)
     * rarely used.
     */
 
-   if(pMe->err) {
-      /* Already in error state. */
-      return;
-   }
-
    /* 0. Sanity check the UsefulOutBuf structure
     *
     * A "counter measure". If magic number is not the right number it
@@ -375,6 +370,11 @@ void UsefulOutBuf_Advance(UsefulOutBuf *pMe, size_t uAmount)
       return;  /* Magic number is wrong due to uninitalization or corrption */
    }
 
+   if(pMe->err) {
+      /* Already in error state. */
+      return;
+   }
+
    /* Make sure valid data is less than buffer size. This would only
     * occur if there was corruption of me, but it is also part of the
     * checks to be sure there is no pointer arithmatic
@@ -409,12 +409,12 @@ void UsefulOutBuf_Advance(UsefulOutBuf *pMe, size_t uAmount)
  */
 UsefulBufC UsefulOutBuf_OutUBuf(UsefulOutBuf *pMe)
 {
-   if(pMe->err) {
+   if(pMe->magic != USEFUL_OUT_BUF_MAGIC) {
+      pMe->err = 1;
       return NULLUsefulBufC;
    }
 
-   if(pMe->magic != USEFUL_OUT_BUF_MAGIC) {
-      pMe->err = 1;
+   if(pMe->err) {
       return NULLUsefulBufC;
    }
 
@@ -442,9 +442,9 @@ UsefulBufC UsefulOutBuf_CopyOut(UsefulOutBuf *pMe, UsefulBuf pDest)
  *
  * Code Reviewers: THIS FUNCTION DOES POINTER MATH
  */
-UsefulBufC UsefulOutBuf_SubString(UsefulOutBuf *pMe,
-                                  const size_t  uStart,
-                                  const size_t  uLen)
+UsefulBufC UsefulOutBuf_OutSubString(UsefulOutBuf *pMe,
+                                     const size_t  uStart,
+                                     const size_t  uLen)
 {
    const UsefulBufC Tmp = UsefulOutBuf_OutUBuf(pMe);
 
@@ -544,34 +544,53 @@ int UsefulOutBuf_Compare(UsefulOutBuf *pMe,
    const uint8_t *pEnd;
    const uint8_t *p1;
    const uint8_t *p2;
+   const uint8_t *p1Start;
+   const uint8_t *p2Start;
    const uint8_t *p1End;
    const uint8_t *p2End;
    int            uComparison;
+   size_t         uComparedLen1;
+   size_t         uComparedLen2;
 
-   pBase = pMe->UB.ptr;
-   pEnd = (const uint8_t *)pBase + pMe->data_len;
-   p1   = pBase + uStart1;
-   p2   = pBase + uStart2;
-   p1End = p1 + uLen1;
-   p2End = p2 + uLen2;
+   pBase   = pMe->UB.ptr;
+   pEnd    = (const uint8_t *)pBase + pMe->data_len;
+   p1Start = pBase + uStart1;
+   p2Start = pBase + uStart2;
+   p1End   = p1Start + uLen1;
+   p2End   = p2Start + uLen2;
 
    uComparison = 0;
-   while(p1 < pEnd && p2 < pEnd && p1 < p1End && p2 < p2End) {
+   for(p1 = p1Start, p2 = p2Start;
+       p1 < pEnd && p2 < pEnd && p1 < p1End && p2 < p2End;
+       p1++, p2++) {
       uComparison = *p2 - *p1;
       if(uComparison != 0) {
-         break;;
+         break;
       }
-      p1++;
-      p2++;
    }
 
-   if(uComparison == 0 && p1 != p1End && p2 != p2End) {
-      if(uLen1 > uLen2) {
+   /* Loop might have terminated because strings were off
+    * the end of the buffer. Compute actual lengths compared.
+    */
+   uComparedLen1 = uLen1;
+   if(p1 >= pEnd) {
+      uComparedLen1 = (size_t)(p1 - p1Start);
+   }
+   uComparedLen2 = uLen2;
+   if(p2 >= pEnd) {
+      uComparedLen2 = (size_t)(p2 - p2Start);
+   }
+
+   if(uComparison == 0) {
+      /* All bytes were equal, now check the lengths */
+      if(uComparedLen2 > uComparedLen1) {
+         /* string 1 is a substring of string 2 */
          uComparison = 1;
-      } else if(uLen2 < uLen1){
+      } else if(uComparedLen1 > uComparedLen2) {
+         /* string 2 is a substring of string 1 */
          uComparison = -1;
-      } else  {
-         return 0;
+      } else {
+         /* do nothing, uComparison already is 0 */
       }
    }
 

src/qcbor_main_encode.c

@@ -1331,5 +1331,5 @@ QCBOREncode_SubString(QCBOREncodeContext *pMe, const size_t uStart)
 
    const size_t uEnd = QCBOREncode_Tell(pMe);
 
-   return UsefulOutBuf_SubString(&(pMe->OutBuf), uStart, uEnd - uStart);
+   return UsefulOutBuf_OutSubString(&(pMe->OutBuf), uStart, uEnd - uStart);
 }