From faa9ca882af7dcd8487fbe192847cfa83b16dc68 Mon Sep 17 00:00:00 2001 From: Liran Alon Date: Thu, 10 Aug 2023 01:37:23 +0300 Subject: [PATCH] alloc: Avoid calling memcpy with a NULL pointer ncclRealloc() is used in the code also to perform the initial allocation. This results in calling memcpy() with a NULL source pointer and 0 copy size. Allegedly this seems harmless, but memcpy is declared to never accept NULL pointers. Which could lead to undefined behaviour. This issue was caught by running nccl-tests with NCCL built with UBSAN: ``` include/alloc.h:68:9: runtime error: null pointer passed as argument 2, which is declared to never be null #0 0x7f867cf80932 in ncclResult_t ncclRealloc(ncclProxyConnection***, unsigned long, unsigned long) include/alloc.h:68 #1 0x7f867cf6c838 in ncclProxyNewConnection /home/ubuntu/nccl/src/proxy.cc:942 #2 0x7f867cf74ba8 in proxyConnInit /home/ubuntu/nccl/src/proxy.cc:1261 #3 0x7f867cf78021 in proxyProgressAsync /home/liran/nccl/src/proxy.cc:1318 #4 0x7f867cf79ff0 in proxyServiceInitOp /home/liran/nccl/src/proxy.cc:1377 #5 0x7f867cf7c052 in ncclProxyService(void*) /home/ubuntu/nccl/src/proxy.cc:1507 #6 0x7f867c3c8608 in start_thread /build/glibc-2.31/nptl/pthread_create.c:477 #7 0x7f867bfba132 in __clone (/lib/x86_64-linux-gnu/libc.so.6) ``` Signed-off-by: Liran Alon --- src/include/alloc.h | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/include/alloc.h b/src/include/alloc.h index caa9da985..94ba97718 100644 --- a/src/include/alloc.h +++ b/src/include/alloc.h @@ -65,8 +65,12 @@ ncclResult_t ncclRealloc(T** ptr, size_t oldNelem, size_t nelem) { WARN("Failed to malloc %ld bytes", nelem*sizeof(T)); return ncclSystemError; } - memcpy(p, oldp, oldNelem*sizeof(T)); - free(oldp); + + if (oldp != NULL) { + memcpy(p, oldp, oldNelem*sizeof(T)); + free(oldp); + } + memset(p+oldNelem, 0, (nelem-oldNelem)*sizeof(T)); *ptr = (T*)p; INFO(NCCL_ALLOC, "Mem Realloc old size %ld, new size %ld pointer %p", oldNelem*sizeof(T), nelem*sizeof(T), *ptr);