From ed28483e7509f0a7a75716f2651f603824fd9817 Mon Sep 17 00:00:00 2001 From: dequis Date: Fri, 23 Oct 2015 04:25:57 -0300 Subject: Fix invalid reads in strsplit_len when splitting on spaces The symptom for this one is randomly getting lines split before the last word, even if there's no need for splitting. Also, this function is only reached if recode is on, and iconv failed (for example, due to an incorrect source charset). Thanks to vague for finding this and providing valgrind logs. The loop that looks for spaces tried to read backwards from the end of the current line, with the end being determined by len. Assuming strsplit_len() with len=400, this meant accessing str[399] in the first iteration. For strings that don't need splitting, this means an invalid read always. If that invalid read happens to hit garbage that has a space character, (len - offset) points after the end of string, which isn't a problem for g_strndup() since it stops at the first null, and no splitting happens. If the garbage doesn't have any spaces, it splits by the last word. This commit avoids that loop entirely if (remaining_len > len). It also changes the way it iterates over the string to be much less confusing. --- src/core/misc.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/core/misc.c b/src/core/misc.c index 88c27255..e209efa1 100644 --- a/src/core/misc.c +++ b/src/core/misc.c @@ -995,25 +995,29 @@ char **strsplit_len(const char *str, int len, gboolean onspace) { char **ret = g_new(char *, 1); int n; - int offset; + int split_offset = 0; + size_t remaining_len = strlen(str); - for (n = 0; *str != '\0'; n++, str += MIN(len - offset, strlen(str))) { - offset = 0; - if (onspace) { + for (n = 0; *str != '\0'; n++) { + split_offset = MIN(len, remaining_len); + if (onspace && remaining_len > len) { /* * Try to find a space to split on and leave * the space on the previous line. */ int i; - for (i = 0; i < len; i++) { - if (str[len-1-i] == ' ') { - offset = i; + for (i = len - 1; i > 0; i--) { + if (str[i] == ' ') { + split_offset = i; break; } } } - ret[n] = g_strndup(str, len - offset); + ret[n] = g_strndup(str, split_offset); ret = g_renew(char *, ret, n + 2); + + str += split_offset; + remaining_len -= split_offset; } ret[n] = NULL; -- cgit v1.2.3 From 8736c12fc95ca977f4fc4a947c760bac651cd3af Mon Sep 17 00:00:00 2001 From: dequis Date: Mon, 9 Nov 2015 06:33:08 -0300 Subject: strsplit_len: use strlen() directly instead of a remaining_len variable --- src/core/misc.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/core/misc.c b/src/core/misc.c index e209efa1..4e8322fa 100644 --- a/src/core/misc.c +++ b/src/core/misc.c @@ -996,11 +996,10 @@ char **strsplit_len(const char *str, int len, gboolean onspace) char **ret = g_new(char *, 1); int n; int split_offset = 0; - size_t remaining_len = strlen(str); for (n = 0; *str != '\0'; n++) { - split_offset = MIN(len, remaining_len); - if (onspace && remaining_len > len) { + split_offset = MIN(len, strlen(str)); + if (onspace && strlen(str) > len) { /* * Try to find a space to split on and leave * the space on the previous line. @@ -1017,7 +1016,6 @@ char **strsplit_len(const char *str, int len, gboolean onspace) ret = g_renew(char *, ret, n + 2); str += split_offset; - remaining_len -= split_offset; } ret[n] = NULL; -- cgit v1.2.3 From b054ade4b90a05d778a64c63c0c834a126884aaa Mon Sep 17 00:00:00 2001 From: dequis Date: Mon, 9 Nov 2015 06:46:40 -0300 Subject: strsplit_len: make it look more like the original version --- src/core/misc.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/core/misc.c b/src/core/misc.c index 4e8322fa..490b5a8f 100644 --- a/src/core/misc.c +++ b/src/core/misc.c @@ -995,10 +995,10 @@ char **strsplit_len(const char *str, int len, gboolean onspace) { char **ret = g_new(char *, 1); int n; - int split_offset = 0; + int offset; - for (n = 0; *str != '\0'; n++) { - split_offset = MIN(len, strlen(str)); + for (n = 0; *str != '\0'; n++, str += offset) { + offset = MIN(len, strlen(str)); if (onspace && strlen(str) > len) { /* * Try to find a space to split on and leave @@ -1007,15 +1007,13 @@ char **strsplit_len(const char *str, int len, gboolean onspace) int i; for (i = len - 1; i > 0; i--) { if (str[i] == ' ') { - split_offset = i; + offset = i; break; } } } - ret[n] = g_strndup(str, split_offset); + ret[n] = g_strndup(str, offset); ret = g_renew(char *, ret, n + 2); - - str += split_offset; } ret[n] = NULL; -- cgit v1.2.3