| Message ID | 20190403153552.23461-7-ben.dooks@codethink.co.uk (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [1/6] validation: ignore temporary ~ files | expand |
On Wed, Apr 03, 2019 at 04:35:52PM +0100, Ben Dooks wrote: > From: Ben Dooks <ben-linux@fluff.org> > > The original update to evaluate.c did the va-arg checking > before the standard checks. This is due to degernate() > removing expr->string so save the string before the loop > and then use it afterwards. > > -> to push back into evaluate.c if no other options available. I think this patch is by far the best thing to do. Even without this ordering problem, get_printf_fmt() is a good thing. I just think it would be better to keep the whole sym->initializer (so you will have an EXPR_STRING with its 'wide' indicator). -- Luc
On 03/04/2019 23:15, Luc Van Oostenryck wrote: > On Wed, Apr 03, 2019 at 04:35:52PM +0100, Ben Dooks wrote: >> From: Ben Dooks <ben-linux@fluff.org> >> >> The original update to evaluate.c did the va-arg checking >> before the standard checks. This is due to degernate() >> removing expr->string so save the string before the loop >> and then use it afterwards. >> >> -> to push back into evaluate.c if no other options available. > > I think this patch is by far the best thing to do. Even > without this ordering problem, get_printf_fmt() is a good thing. > I just think it would be better to keep the whole sym->initializer > (so you will have an EXPR_STRING with its 'wide' indicator). the trouble is, once degenerate() has dealt with the expression for the format string in the standard argument checks, the string has gone (the expr is modified, and the string is nulled out). I tried the following, but got segfaults. diff --git a/evaluate.c b/evaluate.c index 5192c32..f21d6a1 100644 --- a/evaluate.c +++ b/evaluate.c @@ -2691,25 +2691,24 @@ static int parse_format_printf(const char **fmtstring, return 1; } -static const char *get_printf_fmt(struct symbol *fn, struct expression_list *head) +static struct expression *get_printf_fmt(struct symbol *fn, struct expression_list *head) { - struct expression *expr; - const char *fmt_string = NULL; + struct expression *expr, *fmt = NULL; expr = get_expression_n(head, fn->ctype.printf_msg-1); if (!expr) return NULL; if (expr->string && expr->string->length) - fmt_string = expr->string->data; - if (!fmt_string) { + fmt = expr; + if (!fmt) { struct symbol *sym = evaluate_expression(expr); /* attempt to find initialiser for this */ if (sym && sym->initializer && sym->initializer->string) - fmt_string = sym->initializer->string->data; + fmt = sym->initializer; } - return fmt_string; + return fmt; } /* attempt to run through a printf format string and work out the types @@ -2754,7 +2753,8 @@ static int evaluate_arguments(struct symbol *fn, struct expression_list *head) { struct expression *expr; struct symbol_list *argument_types = fn->arguments; - static const char *fmt_string = NULL; + //static const char *fmt_string = NULL; + struct expression *fmt = NULL; struct symbol *argtype; int i = 1; @@ -2762,7 +2762,7 @@ static int evaluate_arguments(struct symbol *fn, struct expression_list *head) * later on in the evaluation loop by degenerate() */ if (Wformat && fn->ctype.printf_va_start) - fmt_string =get_printf_fmt(fn, head); + fmt = get_printf_fmt(fn, head); PREPARE_PTR_LIST(argument_types, argtype); FOR_EACH_PTR (head, expr) { @@ -2802,7 +2802,7 @@ static int evaluate_arguments(struct symbol *fn, struct expression_list *head) FINISH_PTR_LIST(argtype); if (Wformat && fn->ctype.printf_va_start) - evaluate_format_printf(fmt_string, fn, head); + evaluate_format_printf(fmt->string->data, fn, head); return 1; }
On Thu, Apr 04, 2019 at 12:49:41PM +0100, Ben Dooks wrote: > On 03/04/2019 23:15, Luc Van Oostenryck wrote: > > On Wed, Apr 03, 2019 at 04:35:52PM +0100, Ben Dooks wrote: > > > From: Ben Dooks <ben-linux@fluff.org> > > > > > > The original update to evaluate.c did the va-arg checking > > > before the standard checks. This is due to degernate() > > > removing expr->string so save the string before the loop > > > and then use it afterwards. > > > > > > -> to push back into evaluate.c if no other options available. > > > > I think this patch is by far the best thing to do. Even > > without this ordering problem, get_printf_fmt() is a good thing. > > I just think it would be better to keep the whole sym->initializer > > (so you will have an EXPR_STRING with its 'wide' indicator). > > the trouble is, once degenerate() has dealt with the expression for > the format string in the standard argument checks, the string has > gone (the expr is modified, and the string is nulled out). > > I tried the following, but got segfaults. OK. That's unexpected (the array expression should be modified but not its initializer). I'll investigate the cause. -- Luc
diff --git a/evaluate.c b/evaluate.c index a21bc3f..29c3470 100644 --- a/evaluate.c +++ b/evaluate.c @@ -2685,20 +2685,14 @@ static int parse_format_printf(const char **fmtstring, return 1; } -/* attempt to run through a printf format string and work out the types - * it specifies. The format is parsed from the __attribute__(format()) - * in the parser code which stores the positions of the message and arg - * start in the ctype. - */ -static void evaluate_format_printf(struct symbol *fn, struct expression_list *head) +static const char *get_printf_fmt(struct symbol *fn, struct expression_list *head) { - struct format_state state = { }; struct expression *expr; - const char *fmt_string = NULL; + const char *fmt_string; expr = get_expression_n(head, fn->ctype.printf_msg-1); if (!expr) - return; + return NULL; if (expr->string && expr->string->length) fmt_string = expr->string->data; if (!fmt_string) { @@ -2709,6 +2703,23 @@ static void evaluate_format_printf(struct symbol *fn, struct expression_list *he fmt_string = sym->initializer->string->data; } + return fmt_string; +} + +/* attempt to run through a printf format string and work out the types + * it specifies. The format is parsed from the __attribute__(format()) + * in the parser code which stores the positions of the message and arg + * start in the ctype. + */ +static void evaluate_format_printf(const char *fmt_string, struct symbol *fn, struct expression_list *head) +{ + struct format_state state = { }; + struct expression *expr; + + expr = get_expression_n(head, fn->ctype.printf_msg-1); + if (!expr) + return; + state.expr = expr; state.va_start = fn->ctype.printf_va_start; state.arg_index = fn->ctype.printf_va_start; @@ -2737,14 +2748,16 @@ static int evaluate_arguments(struct symbol *fn, struct expression_list *head) { struct expression *expr; struct symbol_list *argument_types = fn->arguments; + static const char *fmt_string = NULL; struct symbol *argtype; int i = 1; /* do this first, otherwise the arugment info may get lost or changed - * later on in the evaluation loop. + * later on in the evaluation loop by degenerate() */ if (Wformat && fn->ctype.printf_va_start) - evaluate_format_printf(fn, head); + fmt_string =get_printf_fmt(fn, head); + PREPARE_PTR_LIST(argument_types, argtype); FOR_EACH_PTR (head, expr) { @@ -2782,6 +2795,10 @@ static int evaluate_arguments(struct symbol *fn, struct expression_list *head) NEXT_PTR_LIST(argtype); } END_FOR_EACH_PTR(expr); FINISH_PTR_LIST(argtype); + + if (Wformat && fn->ctype.printf_va_start) + evaluate_format_printf(fmt_string, fn, head); + return 1; }
From: Ben Dooks <ben-linux@fluff.org> The original update to evaluate.c did the va-arg checking before the standard checks. This is due to degernate() removing expr->string so save the string before the loop and then use it afterwards. -> to push back into evaluate.c if no other options available. --- evaluate.c | 39 ++++++++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 11 deletions(-)