On Mon, Oct 21, 2019 at 09:52:11AM +0900, Junio C Hamano wrote:
> I can sympathize, but I do not think it is worth inventing OPT_U64()
> or adding "int total_i" whose value is assigned to "u64 total" after
> parsing a command line arg with OPT_INTEGER() into the former.
I agree, we should wait for the first real use case where specifying a
larger-than-32bit integer actually makes sense in practice.
> Catching a pointer whose type is not "int*" passed at the third
> position of OPT_INTGER() mechanically may be worth it, though.
> Would Coccinelle be a suitable tool for that kind of thing?
The semantic patch below will do that, but this is one of those "I
don't have the slightest idea what I am doing" patches...
It's output looks like this when applied to an older version without
the big-endian fix upthread:
potential error at apply.c:4982:26:
passing variable 'state -> p_context' of type 'unsigned int' to OPT_INTEGER
OPT_INTEGER expects an int
potential error at builtin/column.c:29:30:
passing variable 'colopts' of type 'unsigned int' to OPT_INTEGER
OPT_INTEGER expects an int
potential error at builtin/column.c:32:24:
passing variable 'copts . nl' of type 'const char *' to OPT_INTEGER
OPT_INTEGER expects an int
potential error at builtin/grep.c:884:38:
passing variable 'opt . pre_context' of type 'unsigned' to OPT_INTEGER
OPT_INTEGER expects an int
potential error at builtin/grep.c:886:37:
passing variable 'opt . post_context' of type 'unsigned' to OPT_INTEGER
OPT_INTEGER expects an int
potential error at builtin/upload-pack.c:28:29:
passing variable 'opts . timeout' of type 'unsigned int' to OPT_INTEGER
OPT_INTEGER expects an int
potential error at t/helper/test-progress.c:42:27:
passing variable 'total' of type 'uint64_t' to OPT_INTEGER
OPT_INTEGER expects an int
https://travis-ci.org/szeder/git/jobs/602423358#L436
I think most of them are harmless, like the number of context lines in
apply and grep, or the timeout in seconds in upload-pack. So I think
the semantic patch should allow 'unsigned' and 'unsigned int' as well.
But note the one in 'builtin/column.c', where we pass a 'const char
*' to OPT_INTEGER. That can't possibly be good; I suspect copy-paste
error and it should have been OPT_STRING.
--- >8 ---
Subject: [PATCH] coccinelle: warn about passing a non-int to parse-options'
OPT_INTEGER
parse-options' OPT_INTEGER wants to parse an integer argument into a
variable of type 'int', and passing e.g. an 'uint64_t' causes troubles
[1].
Add a Coccinelle semantic patch that checks the type of the variable
where the integer argument should be parsed into, and print an error
if that variable is not of type 'int'.
Note that this semantic patch won't result in a proper and applicable
patch, because who knows where that variable of the inappropriate type
is defined. However, the printed error message will still cause our
static analysis CI jobs to fail, drawing our attention to the issue.
TODO: refusing an 'unsigned int' might be unnecessarily harsh...
[1] 11a803d861 (test-progress: fix test failures on big-endian
systems, 2019-10-20)
Signed-off-by: SZEDER Gábor <szeder.dev@xxxxxxxxx>
---
contrib/coccinelle/parse-options.cocci | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
create mode 100644 contrib/coccinelle/parse-options.cocci
diff --git a/contrib/coccinelle/parse-options.cocci b/contrib/coccinelle/parse-options.cocci
new file mode 100644
index 0000000000..e0cddef421
--- /dev/null
+++ b/contrib/coccinelle/parse-options.cocci
@@ -0,0 +1,18 @@
+@ optint @
+identifier opts;
+type T;
+T var;
+expression SHORT, LONG, HELP;
+position p;
+@@
+struct option opts[] = { ..., OPT_INTEGER(SHORT, LONG, &var@p, HELP), ...};
+
+@ script:python @
+p << optint.p;
+var << optint.var;
+vartype << optint.T;
+@@
+if vartype != "int":
+ print "potential error at %s:%s:%s:" % (p[0].file, p[0].line, p[0].column)
+ print " passing variable '%s' of type '%s' to OPT_INTEGER" % (var, vartype)
+ print " OPT_INTEGER expects an int"
--
2.24.0.rc0.502.g7008375535
