Merge branch 'rs/parse-options-precision'

Define .precision to more canned parse-options type to avoid bugs
coming from using a variable with a wrong type to capture the
parsed values.

* rs/parse-options-precision:
  parse-options: add precision handling for OPTION_COUNTUP
  parse-options: add precision handling for OPTION_BITOP
  parse-options: add precision handling for OPTION_NEGBIT
  parse-options: add precision handling for OPTION_BIT
  parse-options: add precision handling for OPTION_SET_INT
  parse-options: add precision handling for PARSE_OPT_CMDMODE
  parse-options: require PARSE_OPT_NOARG for OPTION_BITOP

builtin/am.c

@@ -2406,6 +2406,7 @@ int cmd_am(int argc,
 			.type = OPTION_CALLBACK,
 			.long_name = "show-current-patch",
 			.value = &resume_mode,
+			.precision = sizeof(resume_mode),
 			.argh = "(diff|raw)",
 			.help = N_("show the patch being applied"),
 			.flags = PARSE_OPT_CMDMODE | PARSE_OPT_OPTARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,

builtin/rebase.c

@@ -1128,6 +1128,7 @@ int cmd_rebase(int argc,
 			.short_name = 'n',
 			.long_name = "no-stat",
 			.value = &options.flags,
+			.precision = sizeof(options.flags),
 			.help = N_("do not show diffstat of what changed upstream"),
 			.flags = PARSE_OPT_NOARG,
 			.defval = REBASE_DIFFSTAT,

builtin/update-index.c

@@ -981,6 +981,7 @@ int cmd_update_index(int argc,
 			.type = OPTION_SET_INT,
 			.long_name = "assume-unchanged",
 			.value = &mark_valid_only,
+			.precision = sizeof(mark_valid_only),
 			.help = N_("mark files as \"not changing\""),
 			.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,
 			.defval = MARK_FLAG,
@@ -989,6 +990,7 @@ int cmd_update_index(int argc,
 			.type = OPTION_SET_INT,
 			.long_name = "no-assume-unchanged",
 			.value = &mark_valid_only,
+			.precision = sizeof(mark_valid_only),
 			.help = N_("clear assumed-unchanged bit"),
 			.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,
 			.defval = UNMARK_FLAG,
@@ -997,6 +999,7 @@ int cmd_update_index(int argc,
 			.type = OPTION_SET_INT,
 			.long_name = "skip-worktree",
 			.value = &mark_skip_worktree_only,
+			.precision = sizeof(mark_skip_worktree_only),
 			.help = N_("mark files as \"index-only\""),
 			.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,
 			.defval = MARK_FLAG,
@@ -1005,6 +1008,7 @@ int cmd_update_index(int argc,
 			.type = OPTION_SET_INT,
 			.long_name = "no-skip-worktree",
 			.value = &mark_skip_worktree_only,
+			.precision = sizeof(mark_skip_worktree_only),
 			.help = N_("clear skip-worktree bit"),
 			.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,
 			.defval = UNMARK_FLAG,
@@ -1079,6 +1083,7 @@ int cmd_update_index(int argc,
 			.type = OPTION_SET_INT,
 			.long_name = "fsmonitor-valid",
 			.value = &mark_fsmonitor_only,
+			.precision = sizeof(mark_fsmonitor_only),
 			.help = N_("mark files as fsmonitor valid"),
 			.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,
 			.defval = MARK_FLAG,
@@ -1087,6 +1092,7 @@ int cmd_update_index(int argc,
 			.type = OPTION_SET_INT,
 			.long_name = "no-fsmonitor-valid",
 			.value = &mark_fsmonitor_only,
+			.precision = sizeof(mark_fsmonitor_only),
 			.help = N_("clear fsmonitor valid bit"),
 			.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,
 			.defval = UNMARK_FLAG,

builtin/write-tree.c

@@ -35,6 +35,7 @@ int cmd_write_tree(int argc,
 			.type = OPTION_BIT,
 			.long_name = "ignore-cache-tree",
 			.value = &flags,
+			.precision = sizeof(flags),
 			.help = N_("only useful for debugging"),
 			.flags = PARSE_OPT_HIDDEN | PARSE_OPT_NOARG,
 			.defval = WRITE_TREE_IGNORE_CACHE_TREE,

parse-options.c

@@ -68,6 +68,64 @@ static char *fix_filename(const char *prefix, const char *file)
 		return prefix_filename_except_for_dash(prefix, file);
 }
 
+static int do_get_int_value(const void *value, size_t precision, intmax_t *ret)
+{
+	switch (precision) {
+	case sizeof(int8_t):
+		*ret = *(int8_t *)value;
+		return 0;
+	case sizeof(int16_t):
+		*ret = *(int16_t *)value;
+		return 0;
+	case sizeof(int32_t):
+		*ret = *(int32_t *)value;
+		return 0;
+	case sizeof(int64_t):
+		*ret = *(int64_t *)value;
+		return 0;
+	default:
+		return -1;
+	}
+}
+
+static intmax_t get_int_value(const struct option *opt, enum opt_parsed flags)
+{
+	intmax_t ret;
+	if (do_get_int_value(opt->value, opt->precision, &ret))
+		BUG("invalid precision for option %s", optname(opt, flags));
+	return ret;
+}
+
+static enum parse_opt_result set_int_value(const struct option *opt,
+					   enum opt_parsed flags,
+					   intmax_t value)
+{
+	switch (opt->precision) {
+	case sizeof(int8_t):
+		*(int8_t *)opt->value = value;
+		return 0;
+	case sizeof(int16_t):
+		*(int16_t *)opt->value = value;
+		return 0;
+	case sizeof(int32_t):
+		*(int32_t *)opt->value = value;
+		return 0;
+	case sizeof(int64_t):
+		*(int64_t *)opt->value = value;
+		return 0;
+	default:
+		BUG("invalid precision for option %s", optname(opt, flags));
+	}
+}
+
+static int signed_int_fits(intmax_t value, size_t precision)
+{
+	size_t bits = precision * CHAR_BIT;
+	intmax_t upper_bound = INTMAX_MAX >> (bitsizeof(intmax_t) - bits);
+	intmax_t lower_bound = -upper_bound - 1;
+	return lower_bound <= value && value <= upper_bound;
+}
+
 static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 					  const struct option *opt,
 					  enum opt_parsed flags,
@@ -89,35 +147,55 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 		return opt->ll_callback(p, opt, NULL, unset);
 
 	case OPTION_BIT:
+	{
+		intmax_t value = get_int_value(opt, flags);
 		if (unset)
-			*(int *)opt->value &= ~opt->defval;
+			value &= ~opt->defval;
 		else
-			*(int *)opt->value |= opt->defval;
+			value |= opt->defval;
-		return 0;
+		return set_int_value(opt, flags, value);
+	}
 
 	case OPTION_NEGBIT:
+	{
+		intmax_t value = get_int_value(opt, flags);
 		if (unset)
-			*(int *)opt->value |= opt->defval;
+			value |= opt->defval;
 		else
-			*(int *)opt->value &= ~opt->defval;
+			value &= ~opt->defval;
-		return 0;
+		return set_int_value(opt, flags, value);
+	}
 
 	case OPTION_BITOP:
+	{
+		intmax_t value = get_int_value(opt, flags);
 		if (unset)
 			BUG("BITOP can't have unset form");
-		*(int *)opt->value &= ~opt->extra;
+		value &= ~opt->extra;
-		*(int *)opt->value |= opt->defval;
+		value |= opt->defval;
-		return 0;
+		return set_int_value(opt, flags, value);
+	}
 
 	case OPTION_COUNTUP:
-		if (*(int *)opt->value < 0)
+	{
-			*(int *)opt->value = 0;
+		size_t bits = CHAR_BIT * opt->precision;
-		*(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;
+		intmax_t upper_bound = INTMAX_MAX >> (bitsizeof(intmax_t) - bits);
-		return 0;
+		intmax_t value = get_int_value(opt, flags);
+
+		if (value < 0)
+			value = 0;
+		if (unset)
+			value = 0;
+		else if (value < upper_bound)
+			value++;
+		else
+			return error(_("value for %s exceeds %"PRIdMAX),
+				     optname(opt, flags), upper_bound);
+		return set_int_value(opt, flags, value);
+	}
 
 	case OPTION_SET_INT:
-		*(int *)opt->value = unset ? 0 : opt->defval;
+		return set_int_value(opt, flags, unset ? 0 : opt->defval);
-		return 0;
 
 	case OPTION_STRING:
 		if (unset)
@@ -199,23 +277,7 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 			return error(_("value %s for %s not in range [%"PRIdMAX",%"PRIdMAX"]"),
 				     arg, optname(opt, flags), (intmax_t)lower_bound, (intmax_t)upper_bound);
 
-		switch (opt->precision) {
+		return set_int_value(opt, flags, value);
-		case 1:
-			*(int8_t *)opt->value = value;
-			return 0;
-		case 2:
-			*(int16_t *)opt->value = value;
-			return 0;
-		case 4:
-			*(int32_t *)opt->value = value;
-			return 0;
-		case 8:
-			*(int64_t *)opt->value = value;
-			return 0;
-		default:
-			BUG("invalid precision for option %s",
-			    optname(opt, flags));
-		}
 	}
 	case OPTION_UNSIGNED:
 	{
@@ -266,7 +328,9 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 }
 
 struct parse_opt_cmdmode_list {
-	int value, *value_ptr;
+	intmax_t value;
+	void *value_ptr;
+	size_t precision;
 	const struct option *opt;
 	const char *arg;
 	enum opt_parsed flags;
@@ -280,7 +344,7 @@ static void build_cmdmode_list(struct parse_opt_ctx_t *ctx,
 
 	for (; opts->type != OPTION_END; opts++) {
 		struct parse_opt_cmdmode_list *elem = ctx->cmdmode_list;
-		int *value_ptr = opts->value;
+		void *value_ptr = opts->value;
 
 		if (!(opts->flags & PARSE_OPT_CMDMODE) || !value_ptr)
 			continue;
@@ -292,10 +356,13 @@ static void build_cmdmode_list(struct parse_opt_ctx_t *ctx,
 
 		CALLOC_ARRAY(elem, 1);
 		elem->value_ptr = value_ptr;
-		elem->value = *value_ptr;
+		elem->precision = opts->precision;
+		if (do_get_int_value(value_ptr, opts->precision, &elem->value))
+			optbug(opts, "has invalid precision");
 		elem->next = ctx->cmdmode_list;
 		ctx->cmdmode_list = elem;
 	}
+	BUG_if_bug("invalid 'struct option'");
 }
 
 static char *optnamearg(const struct option *opt, const char *arg,
@@ -317,7 +384,13 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
 	char *opt_name, *other_opt_name;
 
 	for (; elem; elem = elem->next) {
-		if (*elem->value_ptr == elem->value)
+		intmax_t new_value;
+
+		if (do_get_int_value(elem->value_ptr, elem->precision,
+				     &new_value))
+			BUG("impossible: invalid precision");
+
+		if (new_value == elem->value)
 			continue;
 
 		if (elem->opt &&
@@ -327,7 +400,7 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
 		elem->opt = opt;
 		elem->arg = arg;
 		elem->flags = flags;
-		elem->value = *elem->value_ptr;
+		elem->value = new_value;
 	}
 
 	if (result || !elem)
@@ -586,10 +659,14 @@ static void parse_options_check(const struct option *opts)
 		    opts->long_name && !(opts->flags & PARSE_OPT_NONEG))
 			optbug(opts, "OPTION_SET_INT 0 should not be negatable");
 		switch (opts->type) {
-		case OPTION_COUNTUP:
+		case OPTION_SET_INT:
 		case OPTION_BIT:
 		case OPTION_NEGBIT:
-		case OPTION_SET_INT:
+		case OPTION_BITOP:
+		case OPTION_COUNTUP:
+			if (!signed_int_fits(opts->defval, opts->precision))
+				optbug(opts, "has invalid defval");
+			/* fallthru */
 		case OPTION_NUMBER:
 			if ((opts->flags & PARSE_OPT_OPTARG) ||
 			    !(opts->flags & PARSE_OPT_NOARG))

parse-options.h

@@ -172,6 +172,7 @@ struct option {
 	.short_name = (s), \
 	.long_name = (l), \
 	.value = (v), \
+	.precision = sizeof(*v), \
 	.help = (h), \
 	.flags = PARSE_OPT_NOARG|(f), \
 	.callback = NULL, \
@@ -182,6 +183,7 @@ struct option {
 	.short_name = (s), \
 	.long_name = (l), \
 	.value = (v), \
+	.precision = sizeof(*v), \
 	.help = (h), \
 	.flags = PARSE_OPT_NOARG|(f), \
 }
@@ -190,6 +192,7 @@ struct option {
 	.short_name = (s), \
 	.long_name = (l), \
 	.value = (v), \
+	.precision = sizeof(*v), \
 	.help = (h), \
 	.flags = PARSE_OPT_NOARG | (f), \
 	.defval = (i), \
@@ -238,6 +241,7 @@ struct option {
 	.short_name = (s), \
 	.long_name = (l), \
 	.value = (v), \
+	.precision = sizeof(*v), \
 	.help = (h), \
 	.flags = PARSE_OPT_NOARG|PARSE_OPT_NONEG, \
 	.defval = (set), \
@@ -248,6 +252,7 @@ struct option {
 	.short_name = (s), \
 	.long_name = (l), \
 	.value = (v), \
+	.precision = sizeof(*v), \
 	.help = (h), \
 	.flags = PARSE_OPT_NOARG, \
 	.defval = (b), \
@@ -260,6 +265,7 @@ struct option {
 	.short_name = (s), \
 	.long_name = (l), \
 	.value = (v), \
+	.precision = sizeof(*v), \
 	.help = (h), \
 	.flags = PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, \
 	.defval = 1, \
@@ -269,6 +275,7 @@ struct option {
 	.short_name = (s), \
 	.long_name = (l), \
 	.value = (v), \
+	.precision = sizeof(*v), \
 	.help = (h), \
 	.flags = PARSE_OPT_CMDMODE|PARSE_OPT_NOARG|PARSE_OPT_NONEG | (f), \
 	.defval = (i), \

t/helper/test-parse-options.c

@@ -131,6 +131,7 @@ int cmd__parse_options(int argc, const char **argv)
 			.short_name = 'B',
 			.long_name = "no-fear",
 			.value = &boolean,
+			.precision = sizeof(boolean),
 			.help = "be brave",
 			.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,
 			.defval = 1,
@@ -148,9 +149,16 @@ int cmd__parse_options(int argc, const char **argv)
 		OPT_SET_INT(0, "set23", &integer, "set integer to 23", 23),
 		OPT_CMDMODE(0, "mode1", &integer, "set integer to 1 (cmdmode option)", 1),
 		OPT_CMDMODE(0, "mode2", &integer, "set integer to 2 (cmdmode option)", 2),
-		OPT_CALLBACK_F(0, "mode34", &integer, "(3|4)",
+		{
-			"set integer to 3 or 4 (cmdmode option)",
+			.type = OPTION_CALLBACK,
-			PARSE_OPT_CMDMODE, mode34_callback),
+			.long_name = "mode34",
+			.value = &integer,
+			.precision = sizeof(integer),
+			.argh = "(3|4)",
+			.help = "set integer to 3 or 4 (cmdmode option)",
+			.flags = PARSE_OPT_CMDMODE,
+			.callback = mode34_callback,
+		},
 		OPT_CALLBACK('L', "length", &integer, "str",
 			"get length of <str>", length_callback),
 		OPT_FILENAME('F', "file", &file, "set file to <file>"),
@@ -170,6 +178,7 @@ int cmd__parse_options(int argc, const char **argv)
 			.type = OPTION_COUNTUP,
 			.short_name = '+',
 			.value = &boolean,
+			.precision = sizeof(boolean),
 			.help = "same as -b",
 			.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_NODASH,
 		},
@@ -177,6 +186,7 @@ int cmd__parse_options(int argc, const char **argv)
 			.type = OPTION_COUNTUP,
 			.long_name = "ambiguous",
 			.value = &ambiguous,
+			.precision = sizeof(ambiguous),
 			.help = "positive ambiguity",
 			.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,
 		},
@@ -184,6 +194,7 @@ int cmd__parse_options(int argc, const char **argv)
 			.type = OPTION_COUNTUP,
 			.long_name = "no-ambiguous",
 			.value = &ambiguous,
+			.precision = sizeof(ambiguous),
 			.help = "negative ambiguity",
 			.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,
 		},