On 6/18/24 17:33, Wasser Mai wrote:
diffutils-3.10/src/util.c:687:3: alloc_fn: Storage is returned from
allocation function ""xstrdup"".
diffutils-3.10/src/util.c:687:3: var_assign: Assigning: ""color_buf""
= storage returned from ""xstrdup(p)"".
diffutils-3.10/src/util.c:687:3: var_assign: Assigning: ""buf"" = ""color_buf"".
diffutils-3.10/src/util.c:795:1: leaked_storage: Variable ""buf""
going out of scope leaks the storage it points to.
diffutils-3.10/src/util.c:795:1: leaked_storage: Variable
""color_buf"" going out of scope leaks the storage it points to.
Yes I saw that too, but some of that storage might be addressed by the
pointers in color_indicator; see the assignment
"color_indicator[ind_no].string = buf". So that particular diagnostic is
a false positive.
In looking at this code in more detail, though, we should be able to
pacify Coverity (and also fix a true memory leak nearby, which Coverity
didn't notice) by reworking the code to not call malloc either directly
or indirectly via xstrdup. I installed the attached patch to do that.
None of this is a big deal, as hardly anybody uses the --color-palette
option and the true memory leaks are small and rare even when
--color-palette is used.From da0c15f381f59e73dfc5e98403cad86fd707ee54 Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Wed, 19 Jun 2024 23:41:19 -0400
Subject: [PATCH] diff: avoid memory leak with --color-palette
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Problem found indirectly by Coverity, reported by Wasser Mai
(Bug#71535). Although the Coverity report was wrong, there was a
different potential memory leak nearby. Fix the problem by
modifying the string in place, avoiding the need to call malloc.
* src/util.c (color_ext_list, struct color_ext_type):
Remove. Not needed, as the list wasn’t used.
All uses removed.
(get_funky_string): Omit last argument output_count, as it’s
easily calculated by caller. This lets us call this function
when we don’t care about the count.
(color_palette): Now char *, not char const *, since we
now update through it.
(set_color_palette): Likewise.
(parse_diff_color): Process color palette into itself, to avoid
unnecessary malloc and free calls. This pacifies Coverity, saves
a bit of space in the normal case, and avoids a memory leak in
some cases. Do not process the palette twice, as its memory
has been modified and this function had no effect on the
color indicators the second time.
---
src/diff.h | 2 +-
src/util.c | 89 +++++++++++++++---------------------------------------
2 files changed, 26 insertions(+), 65 deletions(-)
diff --git a/src/diff.h b/src/diff.h
index f0723ff..6a97057 100644
--- a/src/diff.h
+++ b/src/diff.h
@@ -472,6 +472,6 @@ enum color_context
extern bool presume_output_tty;
extern void set_color_context (enum color_context color_context);
-extern void set_color_palette (char const *palette);
+extern void set_color_palette (char *palette);
_GL_INLINE_HEADER_END
diff --git a/src/util.c b/src/util.c
index 3807b07..6c65991 100644
--- a/src/util.c
+++ b/src/util.c
@@ -411,37 +411,29 @@ static char const *current_name[2];
static bool currently_recursive;
static bool colors_enabled;
-static struct color_ext_type *color_ext_list = nullptr;
-
struct bin_str
{
idx_t len; /* Number of bytes */
const char *string; /* Pointer to the same */
};
-struct color_ext_type
- {
- struct bin_str ext; /* The extension we're looking for */
- struct bin_str seq; /* The sequence to output when we do */
- struct color_ext_type *next; /* Next in list */
- };
-
/* Parse a string as part of the --palette argument; this may involve
decoding all kinds of escape characters. If equals_end is set an
unescaped equal sign ends the string, otherwise only a : or \0
- does. Set *OUTPUT_COUNT to the number of bytes output. Return
- true if successful.
+ does. Return true if successful.
The resulting string is *not* null-terminated, but may contain
embedded nulls.
+ *dest and *src may point into the same string, in which case *dest
+ must not exceed *src and the string is modified in place.
+
Note that both dest and src are char **; on return they point to
the first free byte after the array and the character that ended
the input string, respectively. */
static bool
-get_funky_string (char **dest, const char **src, bool equals_end,
- idx_t *output_count)
+get_funky_string (char **dest, const char **src, bool equals_end)
{
enum {
ST_GND, ST_BACKSLASH, ST_OCTAL, ST_HEX, ST_CARET, ST_END, ST_ERROR
@@ -450,7 +442,6 @@ get_funky_string (char **dest, const char **src, bool equals_end,
char const *p = *src; /* We don't want to double-indirect */
char *q = *dest; /* the whole darn time. */
- idx_t count = 0; /* No characters counted in yet. */
char num = 0; /* For numerical codes. */
while (state < ST_END)
@@ -481,7 +472,6 @@ get_funky_string (char **dest, const char **src, bool equals_end,
FALLTHROUGH;
default:
*(q++) = *(p++);
- ++count;
break;
}
break;
@@ -545,7 +535,6 @@ get_funky_string (char **dest, const char **src, bool equals_end,
if (state == ST_BACKSLASH)
{
*(q++) = num;
- ++count;
state = ST_GND;
}
++p;
@@ -555,7 +544,6 @@ get_funky_string (char **dest, const char **src, bool equals_end,
if (*p < '0' || *p > '7')
{
*(q++) = num;
- ++count;
state = ST_GND;
}
else
@@ -595,7 +583,6 @@ get_funky_string (char **dest, const char **src, bool equals_end,
break;
default:
*(q++) = num;
- ++count;
state = ST_GND;
break;
}
@@ -606,12 +593,10 @@ get_funky_string (char **dest, const char **src, bool equals_end,
if (*p >= '@' && *p <= '~')
{
*(q++) = *(p++) & 037;
- ++count;
}
else if (*p == '?')
{
*(q++) = 127;
- ++count;
}
else
state = ST_ERROR;
@@ -624,7 +609,6 @@ get_funky_string (char **dest, const char **src, bool equals_end,
*dest = q;
*src = p;
- *output_count = count;
return state != ST_ERROR;
}
@@ -659,10 +643,12 @@ static const char *const indicator_name[] =
};
ARGMATCH_VERIFY (indicator_name, color_indicator);
-static char const *color_palette;
+static char *color_palette;
+/* Set the color palette to PALETTE, a string that set_color_context
+ can modify later. */
void
-set_color_palette (char const *palette)
+set_color_palette (char *palette)
{
color_palette = palette;
}
@@ -670,19 +656,16 @@ set_color_palette (char const *palette)
static void
parse_diff_color (void)
{
- char const *p = color_palette;
+ /* Process color_palette into itself. This saves a bit of memory,
+ and pacifies Coverity. The output is no larger than the input. */
+ char *buf = color_palette;
+ char const *p = buf;
if (p == nullptr || *p == '\0')
return;
+ /* Do not process the color palette twice. */
+ color_palette = nullptr;
char label[] = "??"; /* Indicator label */
- struct color_ext_type *ext = nullptr; /* Extension we are working on */
-
- /* This is an overly conservative estimate, but any possible
- --palette string will *not* generate a color_buf longer than
- itself, so it is a safe way of allocating a buffer in
- advance. */
- char *color_buf = xstrdup (p);
- char *buf = color_buf;
enum parse_state state = PS_START;
while (true)
@@ -697,20 +680,8 @@ parse_diff_color (void)
break;
case '*':
- /* Allocate new extension block and add to head of
- linked list (this way a later definition will
- override an earlier one, which can be useful for
- having terminal-specific defs override global). */
-
- ext = xmalloc (sizeof *ext);
- ext->next = color_ext_list;
- color_ext_list = ext;
-
++p;
- ext->ext.string = buf;
-
- state = (get_funky_string (&buf, &p, true, &ext->ext.len)
- ? PS_4 : PS_FAIL);
+ state = get_funky_string (&buf, &p, true) ? PS_4 : PS_FAIL;
break;
case '\0':
@@ -742,10 +713,13 @@ parse_diff_color (void)
{
if (STREQ (label, indicator_name[ind_no]))
{
- color_indicator[ind_no].string = buf;
- state = (get_funky_string (&buf, &p, false,
- &color_indicator[ind_no].len)
- ? PS_START : PS_FAIL);
+ char *str = buf;
+ if (get_funky_string (&buf, &p, false))
+ {
+ color_indicator[ind_no].string = str;
+ color_indicator[ind_no].len = buf - str;
+ state = PS_START;
+ }
break;
}
}
@@ -755,14 +729,8 @@ parse_diff_color (void)
break;
case PS_4: /* Equal sign after *.ext */
- if (*(p++) == '=')
- {
- ext->seq.string = buf;
- state = (get_funky_string (&buf, &p, false, &ext->seq.len)
- ? PS_START : PS_FAIL);
- }
- else
- state = PS_FAIL;
+ state = (*p++ == '=' && get_funky_string (&buf, &p, false)
+ ? PS_START : PS_FAIL);
break;
case PS_FAIL:
@@ -778,13 +746,6 @@ parse_diff_color (void)
{
error (0, 0,
_("unparsable value for --palette"));
- free (color_buf);
- for (struct color_ext_type *e = color_ext_list; e != nullptr; )
- {
- struct color_ext_type *next = e->next;
- free (e);
- e = next;
- }
colors_enabled = false;
}
}
--
2.34.1
