From 6746454d350ec29455e6f199097b4dc9956c17f9 Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Mon, 13 Jan 2025 14:57:15 +0000 Subject: [PATCH 1/5] Address some Coverity warnings (cherry picked from commit 2a4b40a20c4268f179f17b75d766f4bbc9415464) --- common/os_calls.c | 19 ++++++++++++++++--- common/string_calls.c | 1 + libipm/libipm_recv.c | 2 +- sesman/sesexec/session.c | 4 +--- 4 files changed, 19 insertions(+), 7 deletions(-) diff --git a/common/os_calls.c b/common/os_calls.c index 580ed05060..8a648f427b 100644 --- a/common/os_calls.c +++ b/common/os_calls.c @@ -1805,7 +1805,7 @@ g_set_wait_obj(tintptr obj) return 0; } fd = obj >> 16; - to_write = 4; + to_write = sizeof(buf); written = 0; while (written < to_write) { @@ -1823,12 +1823,13 @@ g_set_wait_obj(tintptr obj) return 1; } } - else if (error > 0) + else if (error > 0 && error <= (int)sizeof(buf)) { written += error; } else { + // Shouldn't get here. return 1; } } @@ -2311,15 +2312,27 @@ g_file_set_cloexec(int fd, int status) struct list * g_get_open_fds(int min, int max) { + if (min < 0) + { + min = 0; + } + struct list *result = list_create(); if (result != NULL) { if (max < 0) { - max = sysconf(_SC_OPEN_MAX); + // sysconf() returns a long. Limit it to a sane value +#define SANE_MAX 100000 + long sc_max = sysconf(_SC_OPEN_MAX); + max = (sc_max < 0) ? 0 : + (sc_max > (long)SANE_MAX) ? SANE_MAX : + sc_max; +#undef SANE_MAX } + // max and min are now both guaranteed to be >= 0 if (max > min) { struct pollfd *fds = g_new0(struct pollfd, max - min); diff --git a/common/string_calls.c b/common/string_calls.c index dca5bc150e..0806ed6ea8 100644 --- a/common/string_calls.c +++ b/common/string_calls.c @@ -444,6 +444,7 @@ g_atoix(const char *str) str += 2; base = 16; } + //coverity[OVERRUN:FALSE] return strtol(str, NULL, base); } diff --git a/libipm/libipm_recv.c b/libipm/libipm_recv.c index 5842a1e0d0..5828e0ebf1 100644 --- a/libipm/libipm_recv.c +++ b/libipm/libipm_recv.c @@ -135,7 +135,7 @@ libipm_msg_in_check_available(struct trans *trans, int *available) enum libipm_status libipm_msg_in_wait_available(struct trans *trans) { - tbus wobj[1]; + tbus wobj[2]; // trans_get_wait_objs() can return at most 2 elements int ocnt = 0; enum libipm_status rv = E_LI_SUCCESS; diff --git a/sesman/sesexec/session.c b/sesman/sesexec/session.c index f9c408205b..1e6d367205 100644 --- a/sesman/sesexec/session.c +++ b/sesman/sesexec/session.c @@ -495,9 +495,6 @@ start_x_server(struct login_info *login_info, unknown_session_type = 1; } - g_free(passwd_file); - passwd_file = NULL; - if (xserver_params == NULL) { LOG(LOG_LEVEL_ERROR, "Out of memory allocating X server params"); @@ -520,6 +517,7 @@ start_x_server(struct login_info *login_info, } /* should not get here */ + g_free(passwd_file); list_delete(xserver_params); LOG(LOG_LEVEL_ERROR, "A fatal error has occurred attempting " "to start the X server on display %u, aborting connection", From 1b344ad5c6f8c69bafb26ac96507fe3ec243a87d Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Fri, 24 Jan 2025 11:21:32 +0000 Subject: [PATCH 2/5] Fix Coverity error triggered in utf8_get_next_char() (cherry picked from commit 0f46b7961cf7c794df72fc3b289f3b956b666a0c) --- common/string_calls.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common/string_calls.c b/common/string_calls.c index 0806ed6ea8..77b5c87d11 100644 --- a/common/string_calls.c +++ b/common/string_calls.c @@ -1191,7 +1191,7 @@ utf8_get_next_char(const char **utf8str_ref, unsigned int *len_ref) /* * Macro used to parse a continuation character * @param cp Character Pointer (incremented on success) - * @param end One character past end of input string + * @param end One character past end of input string, or NULL * @param value The value we're constructing * @param finish_label Where to go in the event of an error */ #define PARSE_CONTINUATION_CHARACTER(cp, end, value, finish_label) \ @@ -1210,7 +1210,7 @@ utf8_get_next_char(const char **utf8str_ref, unsigned int *len_ref) /* Easier to work with unsigned chars and no indirection */ const unsigned char *cp = (const unsigned char *)*utf8str_ref; - const unsigned char *end = (len_ref != NULL) ? cp + *len_ref : cp + 6; + const unsigned char *end = (len_ref != NULL) ? cp + *len_ref : NULL; if (cp == end) { From cc9db9a94765af0d7c71e1f65e4d766f795fed74 Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Fri, 24 Jan 2025 11:27:29 +0000 Subject: [PATCH 3/5] Fix potential double-free (Coverity) xrdp_wm_clear_popup() clears down a popup window, but does not clear the pointer. This can potentially lead to a double-free on the popup window bitmap. (cherry picked from commit b56b2da061870421d3b77ca94969489fdea6c149) --- xrdp/xrdp_wm.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/xrdp/xrdp_wm.c b/xrdp/xrdp_wm.c index 55d6138072..7f9912e3c0 100644 --- a/xrdp/xrdp_wm.c +++ b/xrdp/xrdp_wm.c @@ -1260,7 +1260,7 @@ xrdp_wm_clear_popup(struct xrdp_wm *self) //struct xrdp_bitmap* b; //b = 0; - if (self->popup_wnd != 0) + if (self->popup_wnd != NULL) { //b = self->popup_wnd->popped_from; i = list_index_of(self->screen->child_list, (long)self->popup_wnd); @@ -1269,6 +1269,7 @@ xrdp_wm_clear_popup(struct xrdp_wm *self) self->popup_wnd->width, self->popup_wnd->height); xrdp_bitmap_invalidate(self->screen, &rect); xrdp_bitmap_delete(self->popup_wnd); + self->popup_wnd = NULL; } //xrdp_wm_set_focused(self, b->parent); From 77e0b59f528af8be034054073546772ca1c02fe9 Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Fri, 24 Jan 2025 11:36:40 +0000 Subject: [PATCH 4/5] Fix coverity warning over mem leak in smartcard code (cherry picked from commit 201cdc1aea3958f16d171a8cc493671231c2e376) --- sesman/chansrv/smartcard_pcsc.c | 51 ++++++++++++++++++--------------- 1 file changed, 28 insertions(+), 23 deletions(-) diff --git a/sesman/chansrv/smartcard_pcsc.c b/sesman/chansrv/smartcard_pcsc.c index cfce6dbb98..e4c651fbc0 100644 --- a/sesman/chansrv/smartcard_pcsc.c +++ b/sesman/chansrv/smartcard_pcsc.c @@ -731,6 +731,7 @@ scard_function_list_readers_return(void *user_data, struct trans *con; struct pcsc_list_readers *pcscListReaders; char *msz_readers = NULL; + int rv; LOG_DEVEL(LOG_LEVEL_DEBUG, "scard_function_list_readers_return:"); LOG_DEVEL(LOG_LEVEL_DEBUG, " status 0x%8.8x", status); @@ -787,37 +788,41 @@ scard_function_list_readers_return(void *user_data, out_s = trans_get_out_s(con, 8192); if (out_s == NULL) { - return 1; + rv = 1; } - s_push_layer(out_s, iso_hdr, 8); - out_uint32_le(out_s, llen); - out_uint32_le(out_s, readers); + else { - const char *p = msz_readers; - for (index = 0; index < readers; index++) + s_push_layer(out_s, iso_hdr, 8); + out_uint32_le(out_s, llen); + out_uint32_le(out_s, readers); { - unsigned int slen = strlen(p); - if (slen < 100) - { - out_uint8a(out_s, p, slen); - out_uint8s(out_s, 100 - slen); - } - else + const char *p = msz_readers; + for (index = 0; index < readers; index++) { - out_uint8a(out_s, p, 99); - out_uint8s(out_s, 1); + unsigned int slen = strlen(p); + if (slen < 100) + { + out_uint8a(out_s, p, slen); + out_uint8s(out_s, 100 - slen); + } + else + { + out_uint8a(out_s, p, 99); + out_uint8s(out_s, 1); + } + p += (slen + 1); } - p += (slen + 1); } + out_uint32_le(out_s, status); /* SCARD_S_SUCCESS status */ + s_mark_end(out_s); + bytes = (int) (out_s->end - out_s->data); + s_pop_layer(out_s, iso_hdr); + out_uint32_le(out_s, bytes - 8); + out_uint32_le(out_s, 0x03); /* SCARD_LIST_READERS 0x03 */ + rv = trans_force_write(con); } free(msz_readers); - out_uint32_le(out_s, status); /* SCARD_S_SUCCESS status */ - s_mark_end(out_s); - bytes = (int) (out_s->end - out_s->data); - s_pop_layer(out_s, iso_hdr); - out_uint32_le(out_s, bytes - 8); - out_uint32_le(out_s, 0x03); /* SCARD_LIST_READERS 0x03 */ - return trans_force_write(con); + return rv; } /*****************************************************************************/ From 87f3d4bdfb15fb7240544019033c161224687985 Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Mon, 27 Jan 2025 11:39:38 +0000 Subject: [PATCH 5/5] Fix Coverity warnings in neutrinordp The automated Coverity scan does not currently include neutrinordp Two problems fixed:- 1) MAX_STATIC_CHANNELS at 31 is bigger than freerdp->sessings->channels (16) 2) pamusername in the mod parameters is assumed to be 256 bytes when it is written to. (cherry picked from commit 8196314ad0bd1e68a1e9ee099e69e1d209ee2a11) --- neutrinordp/xrdp-neutrinordp.c | 8 ++++++-- neutrinordp/xrdp-neutrinordp.h | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/neutrinordp/xrdp-neutrinordp.c b/neutrinordp/xrdp-neutrinordp.c index 6574c3facb..375322a52e 100644 --- a/neutrinordp/xrdp-neutrinordp.c +++ b/neutrinordp/xrdp-neutrinordp.c @@ -1790,6 +1790,10 @@ lfreerdp_synchronize(rdpContext *context) static boolean lfreerdp_pre_connect(freerdp *instance) { +#define MAX_FREERDP_CHANNELS \ + (sizeof(instance->settings->channels) / \ + sizeof(instance->settings->channels[0])) + struct mod *mod; int index; int error; @@ -1797,7 +1801,7 @@ lfreerdp_pre_connect(freerdp *instance) int target_chan; int ch_flags; char ch_name[256]; - const char *ch_names[MAX_STATIC_CHANNELS]; + const char *ch_names[MAX_FREERDP_CHANNELS]; char *dst_ch_name; LOG_DEVEL(LOG_LEVEL_INFO, "lfreerdp_pre_connect:"); @@ -1828,7 +1832,7 @@ lfreerdp_pre_connect(freerdp *instance) LOG(LOG_LEVEL_INFO, "Channel '%s' not passed to module", ch_name); } - else if (target_chan < MAX_STATIC_CHANNELS) + else if (target_chan < MAX_FREERDP_CHANNELS) { dst_ch_name = instance->settings->channels[target_chan].name; ch_names[target_chan] = dst_ch_name; diff --git a/neutrinordp/xrdp-neutrinordp.h b/neutrinordp/xrdp-neutrinordp.h index d5dc78a242..b510b059b2 100644 --- a/neutrinordp/xrdp-neutrinordp.h +++ b/neutrinordp/xrdp-neutrinordp.h @@ -229,7 +229,7 @@ struct mod struct bitmap_item bitmap_cache[4][4096]; struct brush_item brush_cache[64]; struct pointer_item pointer_cache[32]; - char pamusername[255]; + char pamusername[256]; int allow_client_experiencesettings; int perf_settings_override_mask; /* Performance bits overridden in ini file */