git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
To: pclouds@gmail.com
Cc: avarab@gmail.com, bturner@atlassian.com, git@vger.kernel.org,
	gitster@pobox.com, tmz@pobox.com
Subject: [PATCH 4/3] parse-options: make compiler check value type mismatch
Date: Fri, 24 May 2019 16:36:11 +0700	[thread overview]
Message-ID: <20190524093611.1165-1-pclouds@gmail.com> (raw)
In-Reply-To: <20190524092442.701-1-pclouds@gmail.com>

There is a disconnect between the actual value type from parse-options
user and the parser itself. This is because we have to store 'value'
pointer as 'void *' and the compiler cannot help point out that the user
is passing a 'long' while the parser expects an 'int'. This could lead
to memory corruption.

In order to spot these type mismatch problems, a dummy inline function
is used to process the 'value' input with the right type, before the
input is stored in 'struct option'. This gives the compiler some context
to start complaining.

The catch though, is that we can only call a function in variable
declaration if it's in automatic scope. Global and static 'struct
option' variables will fail to build after this. But I think this is an
reasonable price to pay, compared to memory corruption.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 For the record, this is what I used to make patch 1/3 (I don't think I
 could just rely on manual code inspection to catch these problems)

 If we are doing something like this, then we have some clean up to do
 first. I think it's worth doing though. But maybe there's a better way?

 parse-options.h | 50 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 31 insertions(+), 19 deletions(-)

diff --git a/parse-options.h b/parse-options.h
index ac6ba8abf9..b6ea0ac66d 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -128,55 +128,67 @@ struct option {
 	intptr_t extra;
 };
 
-#define OPT_BIT_F(s, l, v, h, b, f) { OPTION_BIT, (s), (l), (v), NULL, (h), \
+#define DEFINE_OPT_TYPE_CHECK(name, type)  \
+	static inline void *_opt_ ## name(type *p)	\
+	{						\
+		return p;				\
+	}
+
+DEFINE_OPT_TYPE_CHECK(int, int)
+DEFINE_OPT_TYPE_CHECK(ulong, unsigned long)
+DEFINE_OPT_TYPE_CHECK(string, const char *)
+DEFINE_OPT_TYPE_CHECK(string_list, struct string_list)
+DEFINE_OPT_TYPE_CHECK(timestamp, timestamp_t)
+
+#define OPT_BIT_F(s, l, v, h, b, f) { OPTION_BIT, (s), (l), _opt_int(v), NULL, (h), \
 				      PARSE_OPT_NOARG|(f), NULL, (b) }
-#define OPT_COUNTUP_F(s, l, v, h, f) { OPTION_COUNTUP, (s), (l), (v), NULL, \
+#define OPT_COUNTUP_F(s, l, v, h, f) { OPTION_COUNTUP, (s), (l), _opt_int(v), NULL, \
 				       (h), PARSE_OPT_NOARG|(f) }
-#define OPT_SET_INT_F(s, l, v, h, i, f) { OPTION_SET_INT, (s), (l), (v), NULL, \
+#define OPT_SET_INT_F(s, l, v, h, i, f) { OPTION_SET_INT, (s), (l), _opt_int(v), NULL, \
 					  (h), PARSE_OPT_NOARG | (f), NULL, (i) }
 #define OPT_BOOL_F(s, l, v, h, f)   OPT_SET_INT_F(s, l, v, h, 1, f)
 #define OPT_CALLBACK_F(s, l, v, a, h, f, cb)			\
 	{ OPTION_CALLBACK, (s), (l), (v), (a), (h), (f), (cb) }
-#define OPT_STRING_F(s, l, v, a, h, f)   { OPTION_STRING,  (s), (l), (v), (a), (h), (f) }
-#define OPT_INTEGER_F(s, l, v, h, f)     { OPTION_INTEGER, (s), (l), (v), N_("n"), (h), (f) }
+#define OPT_STRING_F(s, l, v, a, h, f)   { OPTION_STRING,  (s), (l), _opt_string(v), (a), (h), (f) }
+#define OPT_INTEGER_F(s, l, v, h, f)     { OPTION_INTEGER, (s), (l), _opt_int(v), N_("n"), (h), (f) }
 
 #define OPT_END()                   { OPTION_END }
-#define OPT_ARGUMENT(l, v, h)       { OPTION_ARGUMENT, 0, (l), (v), NULL, \
+#define OPT_ARGUMENT(l, v, h)       { OPTION_ARGUMENT, 0, (l), _opt_int(v), NULL, \
 				      (h), PARSE_OPT_NOARG, NULL, 1 }
 #define OPT_GROUP(h)                { OPTION_GROUP, 0, NULL, NULL, NULL, (h) }
 #define OPT_BIT(s, l, v, h, b)      OPT_BIT_F(s, l, v, h, b, 0)
-#define OPT_BITOP(s, l, v, h, set, clear) { OPTION_BITOP, (s), (l), (v), NULL, (h), \
+#define OPT_BITOP(s, l, v, h, set, clear) { OPTION_BITOP, (s), (l), _opt_int(v), NULL, (h), \
 					    PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, \
 					    (set), NULL, (clear) }
-#define OPT_NEGBIT(s, l, v, h, b)   { OPTION_NEGBIT, (s), (l), (v), NULL, \
+#define OPT_NEGBIT(s, l, v, h, b)   { OPTION_NEGBIT, (s), (l), _opt_int(v), NULL, \
 				      (h), PARSE_OPT_NOARG, NULL, (b) }
 #define OPT_COUNTUP(s, l, v, h)     OPT_COUNTUP_F(s, l, v, h, 0)
 #define OPT_SET_INT(s, l, v, h, i)  OPT_SET_INT_F(s, l, v, h, i, 0)
 #define OPT_BOOL(s, l, v, h)        OPT_BOOL_F(s, l, v, h, 0)
-#define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), (v), NULL, \
+#define OPT_HIDDEN_BOOL(s, l, v, h) { OPTION_SET_INT, (s), (l), _opt_int(v), NULL, \
 				      (h), PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1}
-#define OPT_CMDMODE(s, l, v, h, i)  { OPTION_CMDMODE, (s), (l), (v), NULL, \
+#define OPT_CMDMODE(s, l, v, h, i)  { OPTION_CMDMODE, (s), (l), _opt_int(v), NULL, \
 				      (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (i) }
 #define OPT_INTEGER(s, l, v, h)     OPT_INTEGER_F(s, l, v, h, 0)
-#define OPT_MAGNITUDE(s, l, v, h)   { OPTION_MAGNITUDE, (s), (l), (v), \
+#define OPT_MAGNITUDE(s, l, v, h)   { OPTION_MAGNITUDE, (s), (l), _opt_ulong(v), \
 				      N_("n"), (h), PARSE_OPT_NONEG }
 #define OPT_STRING(s, l, v, a, h)   OPT_STRING_F(s, l, v, a, h, 0)
 #define OPT_STRING_LIST(s, l, v, a, h) \
-				    { OPTION_CALLBACK, (s), (l), (v), (a), \
+				    { OPTION_CALLBACK, (s), (l), _opt_string_list(v), (a), \
 				      (h), 0, &parse_opt_string_list }
-#define OPT_UYN(s, l, v, h)         { OPTION_CALLBACK, (s), (l), (v), NULL, \
+#define OPT_UYN(s, l, v, h)         { OPTION_CALLBACK, (s), (l), _opt_int(v), NULL, \
 				      (h), PARSE_OPT_NOARG, &parse_opt_tertiary }
 #define OPT_EXPIRY_DATE(s, l, v, h) \
-	{ OPTION_CALLBACK, (s), (l), (v), N_("expiry-date"),(h), 0,	\
+	{ OPTION_CALLBACK, (s), (l), _opt_timestamp(v), N_("expiry-date"),(h), 0,	\
 	  parse_opt_expiry_date_cb }
 #define OPT_CALLBACK(s, l, v, a, h, f) OPT_CALLBACK_F(s, l, v, a, h, 0, f)
 #define OPT_NUMBER_CALLBACK(v, h, f) \
 	{ OPTION_NUMBER, 0, NULL, (v), NULL, (h), \
 	  PARSE_OPT_NOARG | PARSE_OPT_NONEG, (f) }
-#define OPT_FILENAME(s, l, v, h)    { OPTION_FILENAME, (s), (l), (v), \
+#define OPT_FILENAME(s, l, v, h)    { OPTION_FILENAME, (s), (l), _opt_string(v), \
 				       N_("file"), (h) }
 #define OPT_COLOR_FLAG(s, l, v, h) \
-	{ OPTION_CALLBACK, (s), (l), (v), N_("when"), (h), PARSE_OPT_OPTARG, \
+	{ OPTION_CALLBACK, (s), (l), _opt_int(v), N_("when"), (h), PARSE_OPT_OPTARG, \
 		parse_opt_color_flag_cb, (intptr_t)"always" }
 
 #define OPT_NOOP_NOARG(s, l) \
@@ -301,14 +313,14 @@ int parse_opt_passthru_argv(const struct option *, const char *, int);
 #define OPT__VERBOSE(var, h)  OPT_COUNTUP('v', "verbose", (var), (h))
 #define OPT__QUIET(var, h)    OPT_COUNTUP('q', "quiet",   (var), (h))
 #define OPT__VERBOSITY(var) \
-	{ OPTION_CALLBACK, 'v', "verbose", (var), NULL, N_("be more verbose"), \
+	{ OPTION_CALLBACK, 'v', "verbose", _opt_int(var), NULL, N_("be more verbose"), \
 	  PARSE_OPT_NOARG, &parse_opt_verbosity_cb, 0 }, \
-	{ OPTION_CALLBACK, 'q', "quiet", (var), NULL, N_("be more quiet"), \
+	{ OPTION_CALLBACK, 'q', "quiet", _opt_int(var), NULL, N_("be more quiet"), \
 	  PARSE_OPT_NOARG, &parse_opt_verbosity_cb, 0 }
 #define OPT__DRY_RUN(var, h)  OPT_BOOL('n', "dry-run", (var), (h))
 #define OPT__FORCE(var, h, f) OPT_COUNTUP_F('f', "force",   (var), (h), (f))
 #define OPT__ABBREV(var)  \
-	{ OPTION_CALLBACK, 0, "abbrev", (var), N_("n"),	\
+	{ OPTION_CALLBACK, 0, "abbrev", _opt_int(var), N_("n"),	\
 	  N_("use <n> digits to display SHA-1s"),	\
 	  PARSE_OPT_OPTARG, &parse_opt_abbrev_cb, 0 }
 #define OPT__COLOR(var, h) \
-- 
2.22.0.rc0.322.g2b0371e29a


  parent reply	other threads:[~2019-05-24  9:36 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-24  9:24 [PATCH 0/3] fix diff-parseopt regressions Nguyễn Thái Ngọc Duy
2019-05-24  9:24 ` [PATCH 1/3] diff-parseopt: correct variable types that are used by parseopt Nguyễn Thái Ngọc Duy
2019-05-28 19:23   ` Junio C Hamano
2019-05-24  9:24 ` [PATCH 2/3] diff-parseopt: restore -U (no argument) behavior Nguyễn Thái Ngọc Duy
2019-05-24  9:24 ` [PATCH 3/3] parse-options: check empty value in OPT_INTEGER and OPT_ABBREV Nguyễn Thái Ngọc Duy
2019-05-24  9:36 ` Nguyễn Thái Ngọc Duy [this message]
2019-05-24 17:36 ` [PATCH 0/3] fix diff-parseopt regressions Todd Zullinger
2019-05-24 20:58   ` Todd Zullinger
2019-05-25 10:22   ` Duy Nguyen
2019-05-25 20:48     ` Todd Zullinger
2019-05-29  9:11 ` [PATCH v2 " Nguyễn Thái Ngọc Duy
2019-05-29  9:11   ` [PATCH v2 1/3] diff-parseopt: correct variable types that are used by parseopt Nguyễn Thái Ngọc Duy
2019-05-29 16:43     ` Eric Sunshine
2019-05-29 16:47     ` Todd Zullinger
2019-05-29 19:54       ` Junio C Hamano
2019-05-29  9:11   ` [PATCH v2 2/3] diff-parseopt: restore -U (no argument) behavior Nguyễn Thái Ngọc Duy
2019-05-29  9:11   ` [PATCH v2 3/3] parse-options: check empty value in OPT_INTEGER and OPT_ABBREV Nguyễn Thái Ngọc Duy
2019-05-29 18:03   ` [PATCH v2 0/3] fix diff-parseopt regressions Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190524093611.1165-1-pclouds@gmail.com \
    --to=pclouds@gmail.com \
    --cc=avarab@gmail.com \
    --cc=bturner@atlassian.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=tmz@pobox.com \
    --subject='Re: [PATCH 4/3] parse-options: make compiler check value type mismatch' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).