Fix overlap UB, use (uintptr)

Cast pointers to be compared to (uintptr_t) as in mem_prim_move "Comparing pointers from two separately allocated objects is forbidden as per 6.5.8 C11 except when using (in)equality." See also intel/safestringlib#18 Closes GH #51
rurban · Feb 25, 2018 · d02c888 · d02c888
1 parent 8ed1071
commit d02c888
Show file tree

Hide file tree

Showing 7 changed files with 30 additions and 26 deletions.
diff --git a/build-tools/smoke.sh b/build-tools/smoke.sh
@@ -38,8 +38,8 @@ gmake -s clean
 CC="clang-mp-devel" ./configure --enable-error-dmax && \
     $make -s -j4 check-log && exit
 # try the BSD/darwin memset_s
-CFLAGS="-g -DTEST_MSVCRT" ./configure --enable-shared --enable-debug --enable-unsafe && \
-    make check-log || exit
+#CFLAGS="-g -DTEST_MSVCRT" ./configure --enable-shared --enable-debug --enable-unsafe && \
+#    make check-log || exit
 CC="clang-mp-4.0 -std=c99" \
     ./configure --enable-debug --enable-unsafe --enable-norm-compat && \
     gmake -s -j4 check-log || exit

diff --git a/src/extmem/memccpy_s.c b/src/extmem/memccpy_s.c
@@ -107,11 +107,8 @@ _memccpy_s_chk(void *restrict dest, rsize_t dmax, const void *restrict src,
     CHK_SRC_MEM_NULL_CLEAR("memccpy_s", src)
     CHK_SLEN_MEM_MAX_NOSPC_CLEAR("memccpy_s", n, RSIZE_MAX_MEM)
 
-    /*
-     * overlap is an error
-     */
-    if (unlikely( ((dp >= sp) && (dp < (sp+n))) ||
-                  ((sp > dp) && (sp < (dp+dmax))) )) {
+    /* overlap is disallowed */
+    if (unlikely(CHK_OVRLP(dp,dmax,sp,n))) {
         mem_prim_set(dp, dmax, 0);
         invoke_safe_mem_constraint_handler("memccpy_s: overlap undefined",
                    NULL, ESOVRLP);

diff --git a/src/extmem/memcpy16_s.c b/src/extmem/memcpy16_s.c
@@ -115,11 +115,8 @@ _memcpy16_s_chk (uint16_t *dest, rsize_t dmax,
         }
     }
 
-    /*
-     * overlap is undefined behavior, do not allow
-     */
-    if (unlikely( ((dest > src) && (dest < (src+slen))) ||
-                  ((src > dest) && (src < (dest+dmax/2))) )) {
+    /* overlap is disallowed, but allow dest==src */
+    if (unlikely(CHK_OVRLP_BUTSAME(dest,dmax/2,src,slen))) {
         mem_prim_set(dest, dmax, 0);
         invoke_safe_mem_constraint_handler("memcpy16_s: overlap undefined",
                    (void*)dest, ESOVRLP);

diff --git a/src/extmem/memcpy32_s.c b/src/extmem/memcpy32_s.c
@@ -52,7 +52,7 @@
  * @param[out] dest   pointer to the memory that will be replaced by src.
  * @param[in]  dmax   maximum length of the resulting dest, in bytes
  * @param[in]  src    pointer to the memory that will be copied to dest
- * @param[in]  slen  number of uint32_t's to be copied
+ * @param[in]  slen   number of uint32_t's to be copied
  *
  * @pre   Neither dest nor src shall be a null pointer.
  * @pre   dmax shall not be 0.
@@ -112,11 +112,8 @@ _memcpy32_s_chk (uint32_t *dest, rsize_t dmax, const uint32_t *src, rsize_t slen
         return (RCNEGATE(ESLEMAX));
     }
 
-    /*
-     * overlap is undefined behavior, do not allow
-     */
-    if (unlikely( ((dest > src) && (dest < (src+slen))) ||
-                  ((src > dest) && (src < (dest+dmax/4))) )) {
+    /* overlap is disallowed, but allow dest==src */
+    if (unlikely(CHK_OVRLP_BUTSAME(dest,dmax/4,src,slen))) {
         mem_prim_set(dest, dmax, 0);
         invoke_safe_mem_constraint_handler("memcpy32_s: overlap undefined",
                    (void*)dest, ESOVRLP);

diff --git a/src/mem/memcpy_s.c b/src/mem/memcpy_s.c
@@ -126,9 +126,8 @@ _memcpy_s_chk (void * restrict dest, rsize_t dmax,
         return (RCNEGATE(EOVERFLOW));
     }
 
-    /* overlap is disallowed */
-    if (unlikely( ((dp > sp) && (dp < (sp+slen))) ||
-                  ((sp > dp) && (sp < (dp+dmax))) )) {
+    /* overlap is disallowed, but allow dest==src */
+    if (unlikely(CHK_OVRLP_BUTSAME(dp,dmax,sp,slen))) {
         mem_prim_set(dp, dmax, 0);
         invoke_safe_mem_constraint_handler("memcpy_s: overlap undefined",
                    dest, ESOVRLP);

diff --git a/src/safeclib_private.h b/src/safeclib_private.h
@@ -124,6 +124,10 @@ void abort(void) __attribute__((noreturn));
 # define WANT_C11
 #endif
 
+#ifndef HAVE_UINTPTR_T
+typedef unsigned long uintptr_t;
+#endif
+
 /* mingw 3.4 */
 #ifndef EOVERFLOW
 # ifdef _WIN32
@@ -560,6 +564,19 @@ void abort(void) __attribute__((noreturn));
         CHK_DEST_OVR_BOOL(func, destbos)                                \
     }
 
+/* Comparing pointers from two separately allocated objects is forbidden
+   as per 6.5.8 C11 except when using (in)equality. GH #51 */
+#define CHK_OVRLP(dp,dlen,sp,slen) \
+    (((uintptr_t)dp >= (uintptr_t)sp) &&        \
+     ((uintptr_t)dp < (uintptr_t)(sp+slen))) || \
+    (((uintptr_t)dp < (uintptr_t)sp) &&         \
+     ((uintptr_t)sp < (uintptr_t)(dp+dlen)))
+/* but allow dp==sp */
+#define CHK_OVRLP_BUTSAME(dp,dlen,sp,slen) \
+    (((uintptr_t)dp > (uintptr_t)sp) &&        \
+     ((uintptr_t)dp < (uintptr_t)(sp+slen))) || \
+    (((uintptr_t)dp < (uintptr_t)sp) &&         \
+     ((uintptr_t)sp < (uintptr_t)(dp+dlen)))
 
 /* platform quirks */
 #ifndef SAFECLIB_DISABLE_WCHAR

diff --git a/src/wchar/wmemcpy_s.c b/src/wchar/wmemcpy_s.c
@@ -133,11 +133,8 @@ _wmemcpy_s_chk (wchar_t *dest, rsize_t dlen, const wchar_t *src, rsize_t count,
         }
     }
 
-    /*
-     * overlap is undefined behavior, do not allow
-     */
-    if (unlikely( ((dest > src) && (dest < (src+count))) ||
-                  ((src > dest) && (src < (dest+dlen))) )) {
+    /* overlap is disallowed, but allow dest==src */
+    if (unlikely(CHK_OVRLP_BUTSAME(dest,dlen,src,count))) {
         wmem_set((wmem_type*)dest, (uint32_t)dlen, 0);
         invoke_safe_mem_constraint_handler("wmemcpy_s: overlap undefined",
                    (void*)dest, ESOVRLP);