On 29/10/18 22:08, Luc Van Oostenryck wrote:
On Mon, Oct 29, 2018 at 03:39:47PM +0000, Ben Dooks wrote:
This is an updated set with most of the comments now fixed, but
a few issues that could do with looking at before finalising the
code.
1- the ctype fix doesn't work for prefixed __attrbitue__
See my reply in patch 4/5.
2- do we need an 'ASN-any' type for pointers?
I don't think so (see also patch 4/5).
(the asn-any is for the case printf %p, where the value of the
pointer is printed, and it is not de-refrenced)... maybe using
asn:-1 for "any address space" is a good idea?
The only things to discuss:
- is the printf format parsing good enough?
I think so, certainly as a first step. Maybe just also handle "%i".
See also my suggestion about using a table.
ok, i'm thinking something like:
static void fmt_validate_str(void *data, struct expression **expr,
struct symbol *sym); // todo //
tatic void fmt_validate_ptr(void *data, struct expression **expr, struct
symbol *sym); // todo //
static void fmt_validate_type(void *data, struct expression **expr,
struct symbol *sym)
{
struct symbol *source = degenerate(*expr);
struct symbol *target = data;
/* check source vs. target */
/* note, we need to be fairly strict, long != long long, etc */
}
struct format_string {
const char *fmt,
void (validate)(void *data, struct expression *expr, struct symbol *sym);
void *data
} fmts[] = {
{ "s", .validate = fmt_validate_str, },
{ "p", .validate = fmt_validate_ptr, },
{ "d", .validate = fmt_validate_type, .data = &int_ctype },
{ "ld", .validate = fmt_validate_type, .data = &long_type },
{ "lld", .validate = fmt_validate_type, .data = &llong_type },
{ }
}
where the best match is searched for, and then that function is executed.
- do we need to add a a __attribute__((linux-printk)) for printk fmts?
Good question. I don't think so because in the kernel printk() itself
is declared as using __attribute__((format(printf, 1, 2))).
But a mecanism will be needed in the kernel headers to specifiy
the extended conversion format and their corresponding types.
I was thinking something like:
#pragma extended_printf_formar "%pI4[bl]" const struct sockaddr_in *
but:
* it's just a suggestion
* I'm sure some people will hate to use #pragma
* sparse doesn't have real support of #pragma (but I have some
code for it).
I sort of like that, we should probably start a discussion about
this on the kernel lists..
- should we add a -Wvariadic-format to force address-space safety?
Probably (but with another name).
In shorter term, the warning givi-en by this series should be
conditionalized on -Wformat.
Ok, added.
I think this is getting close to merging.
Yes, it progress well and I'm all in favor to merge early the
basic functionalities. At this stage, what's missing the most
is some testcases. I also really think that the type checking
can't be done by simply using a type (as returned by
evaluate_printf_symbol()), a checking function is needed or
at least use your own generic function instead of trying
do let compatible_argument_type() do this check.
I'll look into how to do this better.
Is there any chance of getting a ptrlist patch to allow fetching
pointers from arbitrary positions in the list, it would mean we
can move the code that's doing the variadic bits out of the main
checking loop altogether.
For when you want to merge, is it ok to send patches again or
do you prefer a mergable branch somewhere?
--
Ben Dooks http://www.codethink.co.uk/
Senior Engineer Codethink - Providing Genius
https://www.codethink.co.uk/privacy.html
