Skip to content

line-log: protect inner strbuf from free #1806

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

line-log: protect inner strbuf from free #1806

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
05c2161
line-log: protect inner strbuf from free
derrickstolee Oct 2, 2024
94d2c03
line-log: remove output_prefix()
peff Oct 3, 2024
e1d825a
diff: modify output_prefix function pointer
peff Oct 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions diff-lib.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -701,7 +701,7 @@ int index_differs_from(struct repository *r,
return (has_changes != 0);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Jeff King via GitGitGadget" <[email protected]> writes:

> From: Jeff King <[email protected]>
>
> The uses of the output_prefix function pointer in the diff_options
> struct is currently difficult to work with by returning a pointer to a
> strbuf. There is only one use that cares about the length of the string,
> which appears to be the only justification of the return type.
>
> We already noticed confusing memory issues around this return type, so
> use a const char * return type to make it clear that the caller does not
> own this string buffer.
>
> Signed-off-by: Jeff King <[email protected]>
> Signed-off-by: Derrick Stolee <[email protected]>
> ---
>  diff-lib.c   | 4 ++--
>  diff.c       | 8 +++-----
>  diff.h       | 2 +-
>  graph.c      | 4 ++--
>  log-tree.c   | 4 ++--
>  range-diff.c | 4 ++--
>  6 files changed, 12 insertions(+), 14 deletions(-)

Very nice.

>  			if (opt->diffopt.output_prefix) {
> -				struct strbuf *msg = NULL;
> +				const char *msg;
>  				msg = opt->diffopt.output_prefix(&opt->diffopt,
>  					opt->diffopt.output_prefix_data);
> -				fwrite(msg->buf, msg->len, 1, opt->diffopt.file);
> +				fwrite(msg, strlen(msg), 1, opt->diffopt.file);
>  			}

OK.  We are not relying on the strbuf being able to have embedded
NUL in the buffer, and this looks very sensible.

Thanks.

}

static struct strbuf *idiff_prefix_cb(struct diff_options *opt UNUSED, void *data)
static const char *idiff_prefix_cb(struct diff_options *opt UNUSED, void *data)
{
return data;
}
Expand All @@ -716,7 +716,7 @@ void show_interdiff(const struct object_id *oid1, const struct object_id *oid2,
opts.output_format = DIFF_FORMAT_PATCH;
opts.output_prefix = idiff_prefix_cb;
strbuf_addchars(&prefix, ' ', indent);
opts.output_prefix_data = &prefix;
opts.output_prefix_data = prefix.buf;
diff_setup_done(&opts);

diff_tree_oid(oid1, oid2, "", &opts);
Expand Down
8 changes: 3 additions & 5 deletions diff.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2315,12 +2315,10 @@ const char *diff_get_color(int diff_use_color, enum color_diff ix)

const char *diff_line_prefix(struct diff_options *opt)
{
struct strbuf *msgbuf;
if (!opt->output_prefix)
return "";
if (opt->output_prefix)
return opt->output_prefix(opt, opt->output_prefix_data);

msgbuf = opt->output_prefix(opt, opt->output_prefix_data);
return msgbuf->buf;
return "";
}

static unsigned long sane_truncate_line(char *line, unsigned long len)
Expand Down
2 changes: 1 addition & 1 deletion diff.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ typedef void (*add_remove_fn_t)(struct diff_options *options,
typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,
struct diff_options *options, void *data);

typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data);
typedef const char *(*diff_prefix_fn_t)(struct diff_options *opt, void *data);

#define DIFF_FORMAT_RAW 0x0001
#define DIFF_FORMAT_DIFFSTAT 0x0002
Expand Down
4 changes: 2 additions & 2 deletions graph.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,7 @@ struct git_graph {
unsigned short default_column_color;
};

static struct strbuf *diff_output_prefix_callback(struct diff_options *opt, void *data)
static const char *diff_output_prefix_callback(struct diff_options *opt, void *data)
{
struct git_graph *graph = data;
static struct strbuf msgbuf = STRBUF_INIT;
Expand All @@ -327,7 +327,7 @@ static struct strbuf *diff_output_prefix_callback(struct diff_options *opt, void
opt->line_prefix_length);
if (graph)
graph_padding_line(graph, &msgbuf);
return &msgbuf;
return msgbuf.buf;
}

static const struct diff_options *default_diffopt;
Expand Down
16 changes: 2 additions & 14 deletions line-log.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -897,16 +897,6 @@ static void print_line(const char *prefix, char first,
fputs("\\ No newline at end of file\n", file);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Patrick Steinhardt wrote (reply to this):

On Thu, Oct 03, 2024 at 11:58:42AM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <[email protected]>
> 
> The output_prefix() method in line-log.c may call a function pointer via
> the diff_options struct. This function pointer returns a strbuf struct
> and then its buffer is passed back. However, that implies that the
> consumer is responsible to free the string. This is especially true
> because the default behavior is to duplicate the empty string.
> 
> The existing functions used in the output_prefix pointer include:
> 
>  1. idiff_prefix_cb() in diff-lib.c. This returns the data pointer, so
>     the value exists across multiple calls.
> 
>  2. diff_output_prefix_callback() in graph.c. This uses a static strbuf
>     struct, so it reuses buffers across calls. These should not be
>     freed.
> 
>  3. output_prefix_cb() in range-diff.c. This is similar to the
>     diff-lib.c case.
> 
> In each case, we should not be freeing this buffer. We can convert the
> output_prefix() function to return a const char pointer and stop freeing
> the result.
> 
> This choice is essentially the opposite of what was done in 394affd46d
> (line-log: always allocate the output prefix, 2024-06-07).
> 
> This was discovered via 'valgrind' while investigating a public report
> of a bug in 'git log --graph -L' [1].
> 
> [1] https://github.com/git-for-windows/git/issues/5185
> 
> This issue would have been caught by the new test, when Git is compiled
> with ASan to catch these double frees.

Thanks a bunch for fixing this! The change looks good to me.

Patrick

}

static char *output_prefix(struct diff_options *opt)
{
if (opt->output_prefix) {
struct strbuf *sb = opt->output_prefix(opt, opt->output_prefix_data);
return sb->buf;
} else {
return xstrdup("");
}
}

static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *range)
{
unsigned int i, j = 0;
Expand All @@ -916,7 +906,7 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang
struct diff_ranges *diff = &range->diff;

struct diff_options *opt = &rev->diffopt;
char *prefix = output_prefix(opt);
const char *prefix = diff_line_prefix(opt);
const char *c_reset = diff_get_color(opt->use_color, DIFF_RESET);
const char *c_frag = diff_get_color(opt->use_color, DIFF_FRAGINFO);
const char *c_meta = diff_get_color(opt->use_color, DIFF_METAINFO);
Expand Down Expand Up @@ -1003,7 +993,6 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang
out:
free(p_ends);
free(t_ends);
free(prefix);
}

/*
Expand All @@ -1012,10 +1001,9 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang
*/
static void dump_diff_hacky(struct rev_info *rev, struct line_log_data *range)
{
char *prefix = output_prefix(&rev->diffopt);
const char *prefix = diff_line_prefix(&rev->diffopt);

fprintf(rev->diffopt.file, "%s\n", prefix);
free(prefix);

while (range) {
dump_diff_hacky_one(rev, range);
Expand Down
4 changes: 2 additions & 2 deletions log-tree.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -923,10 +923,10 @@ int log_tree_diff_flush(struct rev_info *opt)
*/
int pch = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_PATCH;
if (opt->diffopt.output_prefix) {
struct strbuf *msg = NULL;
const char *msg;
msg = opt->diffopt.output_prefix(&opt->diffopt,
opt->diffopt.output_prefix_data);
fwrite(msg->buf, msg->len, 1, opt->diffopt.file);
fwrite(msg, strlen(msg), 1, opt->diffopt.file);
}

/*
Expand Down
4 changes: 2 additions & 2 deletions range-diff.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,7 +480,7 @@ static void patch_diff(const char *a, const char *b,
diff_flush(diffopt);
}

static struct strbuf *output_prefix_cb(struct diff_options *opt UNUSED, void *data)
static const char *output_prefix_cb(struct diff_options *opt UNUSED, void *data)
{
return data;
}
Expand Down Expand Up @@ -508,7 +508,7 @@ static void output(struct string_list *a, struct string_list *b,
opts.flags.suppress_hunk_header_line_count = 1;
opts.output_prefix = output_prefix_cb;
strbuf_addstr(&indent, " ");
opts.output_prefix_data = &indent;
opts.output_prefix_data = indent.buf;
diff_setup_done(&opts);

/*
Expand Down
28 changes: 28 additions & 0 deletions t/t4211-line-log.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,4 +337,32 @@ test_expect_success 'zero-width regex .* matches any function name' '
test_cmp expect actual
'

test_expect_success 'show line-log with graph' '
qz_to_tab_space >expect <<-EOF &&
* $head_oid Modify func2() in file.c
|Z
| diff --git a/file.c b/file.c
| --- a/file.c
| +++ b/file.c
| @@ -6,4 +6,4 @@
| int func2()
| {
| - return F2;
| + return F2 + 2;
| }
* $root_oid Add func1() and func2() in file.c
ZZ
diff --git a/file.c b/file.c
--- /dev/null
+++ b/file.c
@@ -0,0 +6,4 @@
+int func2()
+{
+ return F2;
+}
EOF
git log --graph --oneline -L:func2:file.c >actual &&
test_cmp expect actual
'

test_done
Loading