Skip to content

Commit

Permalink
alloc: Avoid calling memcpy with a NULL pointer
Browse files Browse the repository at this point in the history
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*>(ncclProxyConnection***, unsigned long, unsigned long) include/alloc.h:68
    NVIDIA#1 0x7f867cf6c838 in ncclProxyNewConnection /home/ubuntu/nccl/src/proxy.cc:942
    NVIDIA#2 0x7f867cf74ba8 in proxyConnInit /home/ubuntu/nccl/src/proxy.cc:1261
    NVIDIA#3 0x7f867cf78021 in proxyProgressAsync /home/liran/nccl/src/proxy.cc:1318
    NVIDIA#4 0x7f867cf79ff0 in proxyServiceInitOp /home/liran/nccl/src/proxy.cc:1377
    NVIDIA#5 0x7f867cf7c052 in ncclProxyService(void*) /home/ubuntu/nccl/src/proxy.cc:1507
    NVIDIA#6 0x7f867c3c8608 in start_thread /build/glibc-2.31/nptl/pthread_create.c:477
    NVIDIA#7 0x7f867bfba132 in __clone (/lib/x86_64-linux-gnu/libc.so.6)
```

Signed-off-by: Liran Alon <[email protected]>
  • Loading branch information
liralon committed Aug 10, 2023
1 parent 1b09084 commit 6cfa063
Showing 1 changed file with 6 additions and 2 deletions.
8 changes: 6 additions & 2 deletions src/include/alloc.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down

0 comments on commit 6cfa063

Please sign in to comment.