From 2f46ef27a223fe6653fbb10ec87e9c706d9ed23b Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Mon, 24 Feb 2025 11:36:02 +0000 Subject: [PATCH] Add support for cppcheck 2.17.0 cppcheck 2.17.0 adds checks that a NULL pointer returned from malloc() and calloc() is not used. We do this quite a lot. I've addressed this by adding functions g_malloc_nofail() and g_calloc_nofail() which either allocate memory or abort. functions are now called in places where we are not making these checks. Many of these checks are in test programs or example programs. I've modified the list16 module to handle out-of-memory conditions. --- .github/workflows/build.yml | 2 +- common/list16.c | 99 ++++++++++++++++++++------------- common/list16.h | 6 +- common/os_calls.c | 58 ++++++++++++++++++- common/os_calls.h | 52 ++++++++++++++++- librfxcodec | 2 +- sesman/chansrv/pcsc/xrdp_pcsc.c | 12 ++-- tests/memtest/libmem.c | 17 +++--- xrdpvr/xrdpvr_internal.h | 4 ++ 9 files changed, 190 insertions(+), 62 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 33754d03..ae1e0b17 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -181,7 +181,7 @@ jobs: CC: gcc # This is required to use a version of cppcheck other than that # supplied with the operating system - CPPCHECK_VER: "2.16.0" + CPPCHECK_VER: "2.17.0" CPPCHECK_REPO: https://github.com/danmar/cppcheck.git steps: # Set steps.os.outputs.image to the specific OS (e.g. 'ubuntu20') diff --git a/common/list16.c b/common/list16.c index 5f934a24..77eccd3c 100644 --- a/common/list16.c +++ b/common/list16.c @@ -70,27 +70,64 @@ list16_deinit(struct list16 *self) } /*****************************************************************************/ -void -list16_add_item(struct list16 *self, tui16 item) +/** + * Makes the data array larger + * + * @param self The list + * @return 1 for success, 0 for failure + * + * On failure, the data array is unchanged + */ +static int +expand_array(struct list16 *self) { tui16 *p; int i; - - if (self->count >= self->max_count) + int new_max_count = self->max_count + 4; + if (self->items == self->mitems) { - i = self->max_count; - self->max_count += 4; - p = (tui16 *)g_malloc(sizeof(tui16) * self->max_count, 1); - g_memcpy(p, self->items, sizeof(tui16) * i); - if (self->items != self->mitems) + /* Previous allocation is static. Make a new dynamic allocation */ + p = (tui16 *)malloc(sizeof(tui16) * new_max_count); + if (p == NULL) { - g_free(self->items); + return 0; } - self->items = p; + g_memcpy(p, self->items, sizeof(tui16) * self->max_count); + } + else + { + /* Try to reallocate the existing array */ + p = (tui16 *)realloc(self->items, sizeof(tui16) * new_max_count); + if (p == NULL) + { + return 0; + } + } + + /* Clear the new elements */ + for (i = self->max_count; i < new_max_count; ++i) + { + p[i] = 0; + } + + self->max_count = new_max_count; + self->items = p; + + return 1; +} + +/*****************************************************************************/ +int +list16_add_item(struct list16 *self, tui16 item) +{ + if (self->count >= self->max_count && !expand_array(self)) + { + return 0; } self->items[self->count] = item; self->count++; + return 1; } /*****************************************************************************/ @@ -153,40 +190,22 @@ list16_remove_item(struct list16 *self, int index) } /*****************************************************************************/ -void +int list16_insert_item(struct list16 *self, int index, tui16 item) { - tui16 *p; - int i; - - if (index == self->count) + /* Make sure there's at least one free element in the array */ + if (self->count >= self->max_count && !expand_array(self)) { - list16_add_item(self, item); - return; + return 0; } - if (index >= 0 && index < self->count) + if (index < self->count) { - self->count++; - - if (self->count > self->max_count) - { - i = self->max_count; - self->max_count += 4; - p = (tui16 *)g_malloc(sizeof(tui16) * self->max_count, 1); - g_memcpy(p, self->items, sizeof(tui16) * i); - if (self->items != self->mitems) - { - g_free(self->items); - } - self->items = p; - } - - for (i = (self->count - 2); i >= index; i--) - { - self->items[i + 1] = self->items[i]; - } - - self->items[index] = item; + memmove(&self->items[index + 1], &self->items[index], + (self->count - index) * sizeof(tui16)); } + + self->items[index] = item; + self->count++; + return 1; } diff --git a/common/list16.h b/common/list16.h index eba3f419..85e38c14 100644 --- a/common/list16.h +++ b/common/list16.h @@ -40,7 +40,8 @@ void list16_init(struct list16 *self); void list16_deinit(struct list16 *self); -void +/* Returns != 0 if item added successfully */ +int list16_add_item(struct list16 *self, tui16 item); tui16 list16_get_item(struct list16 *self, int index); @@ -50,7 +51,8 @@ int list16_index_of(struct list16 *self, tui16 item); void list16_remove_item(struct list16 *self, int index); -void +/* Returns != 0 if item added successfully */ +int list16_insert_item(struct list16 *self, int index, tui16 item); #endif diff --git a/common/os_calls.c b/common/os_calls.c index 6bf57ace..21525bbc 100644 --- a/common/os_calls.c +++ b/common/os_calls.c @@ -138,6 +138,9 @@ union sock_info #endif }; +/******************************************************************************/ +static oom_type g_out_of_memory_handler; + /*****************************************************************************/ int g_rm_temp_dir(void) @@ -3971,7 +3974,7 @@ g_save_to_bmp(const char *filename, char *data, int stride_bytes, data -= stride_bytes; if ((depth == 24) && (bits_per_pixel == 32)) { - line = (char *) malloc(file_stride_bytes); + line = (char *) g_malloc_nofail(file_stride_bytes); memset(line, 0, file_stride_bytes); for (index = 0; index < height; index++) { @@ -4276,3 +4279,56 @@ g_qsort(void *base, size_t nitems, size_t size, { qsort(base, nitems, size, compar); } + +/******************************************************************************/ +oom_type +g_set_out_of_memory_handler(oom_type new_handler) +{ + oom_type old_handler = g_out_of_memory_handler; + g_out_of_memory_handler = new_handler; + return old_handler; +} + +/******************************************************************************/ +static void +out_of_memory(void) +{ + if (g_out_of_memory_handler != NULL) + { + g_out_of_memory_handler(); + _exit(1); + } + else + { + abort(); + } +} + +/******************************************************************************/ +void * +g_malloc_nofail(size_t size) +{ + void *res = malloc(size); + if (res == NULL) + { + LOG(LOG_LEVEL_ALWAYS, "g_malloc_nofail() can't allocate %zu bytes", + size); + out_of_memory(); + } + return res; +} + +/******************************************************************************/ +void * +g_calloc_nofail(size_t nmemb, size_t size) +{ + void *res = calloc(nmemb, size); + if (res == NULL) + { + LOG(LOG_LEVEL_ALWAYS, + "g_calloc_nofail() can't allocate %zu * %zu bytes", + nmemb, size); + out_of_memory(); + } + return res; +} diff --git a/common/os_calls.h b/common/os_calls.h index ace30a94..8f457111 100644 --- a/common/os_calls.h +++ b/common/os_calls.h @@ -38,6 +38,9 @@ struct proc_exit_status struct list; +/** Out-of-memory handler type */ +typedef void (*oom_type)(void); + #define g_tcp_can_recv g_sck_can_recv #define g_tcp_can_send g_sck_can_send #define g_tcp_recv g_sck_recv @@ -429,14 +432,57 @@ void g_qsort(void *base, size_t nitems, size_t size, int (*compar)(const void *, const void *)); +/** Set the out-of-memory handler + * @param new_handler Function to call if a memory allocation fails + * @result old handler or NULL if none. + * + * After calling an out-of-memory handler, the program exits + * If no out-of-memory handler is set, the program aborts + * + * Only use this function if there is urgent cleaning-up that must be done + * before the program exits. + */ +oom_type +g_set_out_of_memory_handler(oom_type new_handler); + +/** Allocate memory with error-checking + * + * @param size Size of memory to allocate + * @return Allocated memory + * + * If memory cannot be allocated, the out-of-memory handler is called and + * the program exits + * + * Only use this function if you are unable to handle an out-of-memory + * condition. + */ +void * +g_malloc_nofail(size_t size); + +/** Allocate memory with error-checking + * + * @param Number of elementst to allocate + * @param size Size of each element + * @return Allocated memory + * + * If memory cannot be allocated, the out-of-memory handler is called and + * the program exits + * + * Only use this function if you are unable to handle an out-of-memory + * condition. + */ +void * +g_calloc_nofail(size_t nmemb, size_t size); + /* glib-style wrappers */ #define g_new(struct_type, n_structs) \ - (struct_type *) malloc(sizeof(struct_type) * (n_structs)) + (struct_type *) g_malloc_nofail(sizeof(struct_type) * (n_structs)) #define g_new0(struct_type, n_structs) \ - (struct_type *) calloc((n_structs), sizeof(struct_type)) + (struct_type *) g_calloc_nofail((n_structs), sizeof(struct_type)) /* remove these when no longer used */ -#define g_malloc(_size, _zero) (_zero ? calloc(1, _size) : malloc(_size)) +#define g_malloc(_size, _zero) \ + (_zero ? g_calloc_nofail(1, _size) : g_malloc_nofail(_size)) #define g_free free #define g_memset memset #define g_memcpy memcpy diff --git a/librfxcodec b/librfxcodec index 26e40b29..962b3a34 160000 --- a/librfxcodec +++ b/librfxcodec @@ -1 +1 @@ -Subproject commit 26e40b29d05877e926f977c4b40b2a44bbb07216 +Subproject commit 962b3a34ff44b4eb333f7d7ea2a2ef72c976dd8b diff --git a/sesman/chansrv/pcsc/xrdp_pcsc.c b/sesman/chansrv/pcsc/xrdp_pcsc.c index 220c97c7..46c047c5 100644 --- a/sesman/chansrv/pcsc/xrdp_pcsc.c +++ b/sesman/chansrv/pcsc/xrdp_pcsc.c @@ -672,7 +672,7 @@ SCardStatus(SCARDHANDLE hCard, LPSTR mszReaderName, LPDWORD pcchReaderLen, LLOGLN(10, (" cbAtrLen %d", (int)*pcbAtrLen)); cchReaderLen = *pcchReaderLen; - msg = (char *) malloc(8192); + msg = (char *) g_malloc_nofail(8192); SET_UINT32(msg, 0, hCard); SET_UINT32(msg, 4, cchReaderLen); SET_UINT32(msg, 8, *pcbAtrLen); @@ -761,7 +761,7 @@ SCardGetStatusChange(SCARDCONTEXT hContext, DWORD dwTimeout, LLOGLN(0, ("SCardGetStatusChange: error, not connected")); return SCARD_F_INTERNAL_ERROR; } - msg = (char *) malloc(8192); + msg = (char *) g_malloc_nofail(8192); SET_UINT32(msg, 0, hContext); SET_UINT32(msg, 4, dwTimeout); SET_UINT32(msg, 8, cReaders); @@ -910,7 +910,7 @@ SCardControl(SCARDHANDLE hCard, DWORD dwControlCode, LPCVOID pbSendBuffer, dwControlCode = dwControlCode | (49 << 16); LLOGLN(10, (" MS dwControlCode 0x%8.8d", (int)dwControlCode)); - msg = (char *) malloc(8192); + msg = (char *) g_malloc_nofail(8192); offset = 0; SET_UINT32(msg, offset, hCard); offset += 4; @@ -986,7 +986,7 @@ SCardTransmit(SCARDHANDLE hCard, const SCARD_IO_REQUEST *pioSendPci, LLOGLN(10, (" pioRecvPci->dwProtocol %d", (int)(pioRecvPci->dwProtocol))); LLOGLN(10, (" pioRecvPci->cbPciLength %d", (int)(pioRecvPci->cbPciLength))); } - msg = (char *) malloc(8192); + msg = (char *) g_malloc_nofail(8192); offset = 0; SET_UINT32(msg, offset, hCard); offset += 4; @@ -1124,7 +1124,7 @@ SCardListReaders(SCARDCONTEXT hContext, LPCSTR mszGroups, LPSTR mszReaders, { *pcchReaders = 0; } - msg = (char *) malloc(8192); + msg = (char *) g_malloc_nofail(8192); offset = 0; SET_UINT32(msg, offset, hContext); offset += 4; @@ -1171,7 +1171,7 @@ SCardListReaders(SCARDCONTEXT hContext, LPCSTR mszGroups, LPSTR mszReaders, offset += 4; LLOGLN(10, ("SCardListReaders: mszReaders %p pcchReaders %p num_readers %d", mszReaders, pcchReaders, num_readers)); - reader_names = (char *) malloc(8192); + reader_names = (char *) g_malloc_nofail(8192); reader_names_index = 0; for (index = 0; index < num_readers; index++) { diff --git a/tests/memtest/libmem.c b/tests/memtest/libmem.c index 960eadc3..2c835f0f 100644 --- a/tests/memtest/libmem.c +++ b/tests/memtest/libmem.c @@ -9,6 +9,7 @@ #include "libmem.h" #include "log.h" +#include "os_calls.h" #define ALIGN_BY 32 #define ALIGN_BY_M1 (ALIGN_BY - 1) @@ -78,12 +79,12 @@ libmem_init(unsigned int addr, int bytes) struct mem_info *self; struct mem_item *mi; - self = (struct mem_info *)malloc(sizeof(struct mem_info)); + self = (struct mem_info *)g_malloc_nofail(sizeof(struct mem_info)); memset(self, 0, sizeof(struct mem_info)); self->addr = addr; self->bytes = bytes; //self->flags = 1; - mi = (struct mem_item *)malloc(sizeof(struct mem_item)); + mi = (struct mem_item *)g_malloc_nofail(sizeof(struct mem_item)); memset(mi, 0, sizeof(struct mem_item)); mi->addr = addr; mi->bytes = bytes; @@ -129,7 +130,7 @@ libmem_add_used_item(struct mem_info *self, unsigned int addr, int bytes) if (self->used_head == 0) { /* add first item */ - new_mi = (struct mem_item *)malloc(sizeof(struct mem_item)); + new_mi = (struct mem_item *)g_malloc_nofail(sizeof(struct mem_item)); memset(new_mi, 0, sizeof(struct mem_item)); new_mi->addr = addr; new_mi->bytes = bytes; @@ -144,7 +145,7 @@ libmem_add_used_item(struct mem_info *self, unsigned int addr, int bytes) if (mi->addr > addr) { /* add before */ - new_mi = (struct mem_item *)malloc(sizeof(struct mem_item)); + new_mi = (struct mem_item *)g_malloc_nofail(sizeof(struct mem_item)); memset(new_mi, 0, sizeof(struct mem_item)); new_mi->addr = addr; new_mi->bytes = bytes; @@ -167,7 +168,7 @@ libmem_add_used_item(struct mem_info *self, unsigned int addr, int bytes) if (!added) { /* add last */ - new_mi = (struct mem_item *)malloc(sizeof(struct mem_item)); + new_mi = (struct mem_item *)g_malloc_nofail(sizeof(struct mem_item)); memset(new_mi, 0, sizeof(struct mem_item)); new_mi->addr = addr; new_mi->bytes = bytes; @@ -193,7 +194,7 @@ libmem_add_free_item(struct mem_info *self, unsigned int addr, int bytes) if (self->free_head == 0) { /* add first item */ - new_mi = (struct mem_item *)malloc(sizeof(struct mem_item)); + new_mi = (struct mem_item *)g_malloc_nofail(sizeof(struct mem_item)); memset(new_mi, 0, sizeof(struct mem_item)); new_mi->addr = addr; new_mi->bytes = bytes; @@ -230,7 +231,7 @@ libmem_add_free_item(struct mem_info *self, unsigned int addr, int bytes) return 0; } /* add before */ - new_mi = (struct mem_item *)malloc(sizeof(struct mem_item)); + new_mi = (struct mem_item *)g_malloc_nofail(sizeof(struct mem_item)); memset(new_mi, 0, sizeof(struct mem_item)); new_mi->addr = addr; new_mi->bytes = bytes; @@ -253,7 +254,7 @@ libmem_add_free_item(struct mem_info *self, unsigned int addr, int bytes) if (!added) { /* add last */ - new_mi = (struct mem_item *)malloc(sizeof(struct mem_item)); + new_mi = (struct mem_item *)g_malloc_nofail(sizeof(struct mem_item)); memset(new_mi, 0, sizeof(struct mem_item)); new_mi->addr = addr; new_mi->bytes = bytes; diff --git a/xrdpvr/xrdpvr_internal.h b/xrdpvr/xrdpvr_internal.h index 90f87361..ba9a0b6a 100644 --- a/xrdpvr/xrdpvr_internal.h +++ b/xrdpvr/xrdpvr_internal.h @@ -90,6 +90,10 @@ typedef struct stream do \ { \ (_s) = (STREAM *) calloc(1, sizeof(STREAM)); \ + if (!(_s)) \ + { \ + abort(); \ + } \ (_s)->data = (u8 *) calloc(1, (_len)); \ (_s)->p = (_s)->data; \ (_s)->size = (_len); \