From a937c8ac5a9ce7adea388d6f03c42850d40e1df8 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Fri, 18 Nov 2022 16:18:55 +0000 Subject: [PATCH 1/3] giochannel: Clarify assertions in g_io_channel_write_chars() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit How the assertions handled the case of `buf != NULL && count == -1` and `buf == NULL && count == -1` were a bit fragile. In the former case, the `strlen (buf)` was assigned to `count`, which is signed. If, somehow, `buf` was huge, `count` would end up wrapping around to a negative number. Avoid that by assigning directly to `count_unsigned`. In the latter case, `count_unsigned` would be set to `-1` which would wrap around. The error would then be caught by the precondition on `buf != NULL`, but it seems like that could have been a happy accident rather than something intentional. Change it to an explicit precondition which only allows `buf == NULL` iff `count == 0`. Spotted while reading through static analysis issues, although the analyser didn’t explicitly flag this up as an issue. Signed-off-by: Philip Withnall --- glib/giochannel.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/glib/giochannel.c b/glib/giochannel.c index 25baf42c9..e0b0ed515 100644 --- a/glib/giochannel.c +++ b/glib/giochannel.c @@ -2208,13 +2208,15 @@ g_io_channel_write_chars (GIOChannel *channel, gssize wrote_bytes = 0; g_return_val_if_fail (channel != NULL, G_IO_STATUS_ERROR); + g_return_val_if_fail (buf != NULL || count == 0, G_IO_STATUS_ERROR); g_return_val_if_fail ((error == NULL) || (*error == NULL), G_IO_STATUS_ERROR); g_return_val_if_fail (channel->is_writeable, G_IO_STATUS_ERROR); - if ((count < 0) && buf) - count = strlen (buf); - count_unsigned = count; + if (count < 0) + count_unsigned = strlen (buf); + else + count_unsigned = count; if (count_unsigned == 0) { @@ -2223,8 +2225,7 @@ g_io_channel_write_chars (GIOChannel *channel, return G_IO_STATUS_NORMAL; } - g_return_val_if_fail (buf != NULL, G_IO_STATUS_ERROR); - g_return_val_if_fail (count_unsigned > 0, G_IO_STATUS_ERROR); + g_assert (count_unsigned > 0); /* Raw write case */ From b2dd6d90534071157da39be349a1e58a1db21634 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 22 Nov 2022 14:06:07 +0000 Subject: [PATCH 2/3] giochannel: Fix an invalid comparison Since commit 2f9e6e977a, `count` has been used here incorrectly: after `count_unsigned` is initialised, `count` should no longer be used as it might be unhelpfully negative. Fix this to correctly use `count_unsigned`. Signed-off-by: Philip Withnall --- glib/giochannel.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/glib/giochannel.c b/glib/giochannel.c index e0b0ed515..9f19ad6cf 100644 --- a/glib/giochannel.c +++ b/glib/giochannel.c @@ -2267,7 +2267,7 @@ g_io_channel_write_chars (GIOChannel *channel, if (!channel->write_buf) channel->write_buf = g_string_sized_new (channel->buf_size); - while (wrote_bytes < count) + while (wrote_bytes < count_unsigned) { gsize space_in_buf; From a03160adf304aa523ddf29fec916477c16c17cbf Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 22 Nov 2022 14:15:11 +0000 Subject: [PATCH 3/3] giochannel: Fix incorrect use of a signed gsize when unsigned will do MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The value of `wrote_bytes` will never be negative, so there’s no need to store it in a signed type. Add a couple of assertions to validate that it never decreases and hence can never go negative. Signed-off-by: Philip Withnall --- glib/giochannel.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/glib/giochannel.c b/glib/giochannel.c index 9f19ad6cf..a16891f4f 100644 --- a/glib/giochannel.c +++ b/glib/giochannel.c @@ -2205,7 +2205,7 @@ g_io_channel_write_chars (GIOChannel *channel, { gsize count_unsigned; GIOStatus status; - gssize wrote_bytes = 0; + gsize wrote_bytes = 0; g_return_val_if_fail (channel != NULL, G_IO_STATUS_ERROR); g_return_val_if_fail (buf != NULL || count == 0, G_IO_STATUS_ERROR); @@ -2313,7 +2313,11 @@ g_io_channel_write_chars (GIOChannel *channel, if (!channel->encoding) { - gssize write_this = MIN (space_in_buf, count_unsigned - wrote_bytes); + gsize write_this = MIN (space_in_buf, count_unsigned - wrote_bytes); + + /* g_string_append_len() takes a gssize, so don’t overflow it*/ + if (write_this > G_MAXSSIZE) + write_this = G_MAXSSIZE; g_string_append_len (channel->write_buf, buf, write_this); buf += write_this; @@ -2476,7 +2480,10 @@ reconvert: g_warning ("Illegal sequence due to partial character " "at the end of a previous write."); else - wrote_bytes += from_buf_len - left_len - from_buf_old_len; + { + g_assert (from_buf_len >= left_len + from_buf_old_len); + wrote_bytes += from_buf_len - left_len - from_buf_old_len; + } if (bytes_written) *bytes_written = wrote_bytes; channel->partial_write_buf[0] = '\0';