fix parse param bug and set default when overflow

[guodashun]

1. In this patch, we check if the value is overflow before parsing the value with its unit. we replaced sscanf with strtoul cause the strtoul has clear errno ERANGE for overflow case.
2. In this patch, we deleted the wrong logic unit_matched=0. When we set the param with a high level unit (e.g. PAGE_CE_THRESHOLD=1m), the real value we expect is 1000000(1*1000*1000), but now we only match the unit 'm' and multiply 1000 once, so the value we get is 1000 rather than 1000000. In this patch we fix the bug.
3. When the value is overflow, the sscanf will produce Undefined Behavior (https://man7.org/linux/man-pages/man3/sscanf.3.html#BUGS). The final value after being truncated is confusing. So in this patch we will set to default value when the value is overflow.

[mchehab]

It seems that the patch is incomplete:

ras-page-isolation.c:43:10: error: ‘struct isolation’ has no member named ‘var’; did you mean ‘val’?
   43 |         .var = 50,
      |          ^~~
      |          val
ras-page-isolation.c:43:16: warning: excess elements in struct initializer
   43 |         .var = 50,
      |                ^~
ras-page-isolation.c:43:16: note: (near initialization for ‘threshold’)
ras-page-isolation.c:51:10: error: ‘struct isolation’ has no member named ‘var’; did you mean ‘val’?
   51 |         .var = 50,
      |          ^~~
      |          val
ras-page-isolation.c:51:16: warning: excess elements in struct initializer
   51 |         .var = 50,
      |                ^~
ras-page-isolation.c:51:16: note: (near initialization for ‘row_threshold’)
ras-page-isolation.c:59:10: error: ‘struct isolation’ has no member named ‘var’; did you mean ‘val’?
   59 |         .var = 86400,
      |          ^~~
      |          val
ras-page-isolation.c:59:16: warning: excess elements in struct initializer
   59 |         .var = 86400,
      |                ^~~~~
ras-page-isolation.c:59:16: note: (near initialization for ‘cycle’)
ras-page-isolation.c:67:10: error: ‘struct isolation’ has no member named ‘var’; did you mean ‘val’?
   67 |         .var = 86400,
      |          ^~~
      |          val
ras-page-isolation.c:67:16: warning: excess elements in struct initializer
   67 |         .var = 86400,
      |                ^~~~~
ras-page-isolation.c:67:16: note: (near initialization for ‘row_cycle’)

Besides that:

1. scanf errors can be checked by looking on its return code:

   if (scanf("sscanf(config->env, "%lu", &value) != 1) {
      // ERROR!
   }
   

2. with the addition of "var" field, can we get rid of "env" as they both seem to be having the same information?

[mchehab]

Ah, when submitting the new version, please add a proper description and place your name at the from/SOB, like:

From: Guo Dashun <[email protected]>
Date: Mon, 9 Dec 2024 19:41:54 +0800
Subject: [PATCH] fix parse param bug and set default when overflow

1. Check if the value is overflow before parsing the value with its
unit, replacing sscanf with strtoul cause the strtoul has clear errno
ERANGE for overflow case;

2. Delete the wrong logic unit_matched=0. When the param is set with a
high level unit (e.g. PAGE_CE_THRESHOLD=1m), the real expected value is
1000000(1*1000*1000), but currently, it only matches the unit 'm' and
multiply 1000 once, so the value we get is 1000 rather than 1000000;

3. Fix an overflow bug, as sscanf produces Undefined Behavior
(https://man7.org/linux/man-pages/man3/sscanf.3.html#BUGS). The final
value after being truncated is confusing. So set it to default value
on overflow.

Signed-off-by: Guo Dashun <[email protected]>

[guodashun]

1. sorry for the typo that 'val' to 'var'
2. we cannot get rid of "env" because the 'env' is the value user want to set and 'val' is the default value and the final value after processing. And for keeping original function, we need to keep the 'env' until we finish the final info printing. e.g. snprintf(threshold_string, sizeof(threshold_string), "%s%s", threshold.env, threshold.unit);

[mchehab]

1. sorry for the typo that 'val' to 'var'

   2. we cannot get rid of "env" because the 'env' is the value user want to set and 'val' is the default value and the final value after processing. And for keeping original function, we need to keep the 'env' until we finish the final info printing. e.g. snprintf(threshold_string, sizeof(threshold_string), "%s%s", threshold.env, threshold.unit);

This would do the same: snprintf(threshold_string, sizeof(threshold_string), "%d%s", threshold.var, threshold.unit);

The only thing is that 86400 seconds would need to be stored there as {.var = 1, .unit = "d"}

[mchehab]

To be clearer, I'm talking about what would be stored on things like these:

static struct isolation cycle = {
        .name = "PAGE_CE_REFRESH_CYCLE",
        .units = cycle_units,
        .env = "24h",
        .unit = "h",
};

That you're already adding the same default in seconds, via .var variable:

ras-page-isolation.c:67:16: warning: excess elements in struct initializer
   67 |         .var = 86400,
      |             

The above could be stored, instead, as:

static struct isolation cycle = {
       .name = "PAGE_CE_REFRESH_CYCLE",
       .units = cycle_units,
       .var = 24,
       .unit = "h",
};

(where .unit could either be seconds, minutes, hours or days - not sure what would work best )

[mchehab]

on a separate but related issue, "var" is a terrible name (and "env"). The default value should be called "default". The one in use I would call "value".

[guodashun]

To be clearer, I'm talking about what would be stored on things like these:

static struct isolation cycle = {
        .name = "PAGE_CE_REFRESH_CYCLE",
        .units = cycle_units,
        .env = "24h",
        .unit = "h",
};

That you're already adding the same default in seconds, via .var variable:

ras-page-isolation.c:67:16: warning: excess elements in struct initializer
   67 |         .var = 86400,
      |             

The above could be stored, instead, as:

static struct isolation cycle = {
       .name = "PAGE_CE_REFRESH_CYCLE",
       .units = cycle_units,
       .var = 24,
       .unit = "h",
};

(where .unit could either be seconds, minutes, hours or days - not sure what would work best )

Under normal circumstances, you’re totally right. But here’s the edge case I’m thinking about: If I set a crazy huge value like 213503982334062d, here’s what happens in the code logic. First, it doesn’t hit the maximum int limit, so no truncation. Then when converting it to the default unit, boom—(213503982334062d to 18446744073662956800s )suddenly the converted value exceeds the max limit.

But here’s the catch: If I only use either 'env' or 'unit' to store the working value, I completely lose track of the original value the user actually input. That means when I need to show error messages later, I can’t tell the user 'Hey your original 213503982334062d was invalid'—I’d only have the messed-up converted value.

So here’s the thing—we can’t have both accurate value processing AND accurate error reporting if we only use one storage variable. That’s why I ended up keeping both 'env' (for the original user input) and 'val' (for the working value). This way we handle the math correctly while still being able to point to exactly what the user typed when things go wrong.

[mchehab]

The above could be stored, instead, as:

static struct isolation cycle = {
       .name = "PAGE_CE_REFRESH_CYCLE",
       .units = cycle_units,
       .var = 24,
       .unit = "h",
};

(where .unit could either be seconds, minutes, hours or days - not sure what would work best )

Under normal circumstances, you’re totally right. But here’s the edge case I’m thinking about: If I set a crazy huge value like 213503982334062d, here’s what happens in the code logic. First, it doesn’t hit the maximum int limit, so no truncation. Then when converting it to the default unit, boom—(213503982334062d to 18446744073662956800s )suddenly the converted value exceeds the max limit.

But here’s the catch: If I only use either 'env' or 'unit' to store the working value, I completely lose track of the original value the user actually input. That means when I need to show error messages later, I can’t tell the user 'Hey your original 213503982334062d was invalid'—I’d only have the messed-up converted value.

So here’s the thing—we can’t have both accurate value processing AND accurate error reporting if we only use one storage variable. That’s why I ended up keeping both 'env' (for the original user input) and 'val' (for the working value). This way we handle the math correctly while still being able to point to exactly what the user typed when things go wrong.

The issue is because we're declaring things like:

struct isolation {
	char			*name;
	char			*env;
	const struct config	*units;
	unsigned long	val;
	bool			overflow;
	char			*unit;
};

static struct isolation cycle = {
	.name = "PAGE_CE_REFRESH_CYCLE",
	.units = cycle_units,
	.env = "24h",
	.unit = "h",
};

And then let user change what's there. I'm not a big fan of having a non-const default structure, as this makes it harder to identify changes. Anyway, the imutable data including the default value should be const. Also, the name seems to be const as well.

IMO, the best solution is to split const from non-const data, e.g.:

struct isolation_props {
	char		*name;
	unsigned long	default;
	char		*default_unit;
	struct config	*units;
}

struct isolation {
	const struct isolation_props props;

	unsigned long	value;
	const char	*unit;
};

static struct isolation cycle = {
	.props = {
		.name = "PAGE_CE_REFRESH_CYCLE",
		.units = cycle_units,
		.default = 24 * 60 * 60 /* seconds */,
	}
};

And ensure that value and unit will be properly set based on the default values and the user input.

Please notice that I removed overflow, as, on overflow, the logic should set to default.

[guodashun]

The above could be stored, instead, as:

static struct isolation cycle = {
       .name = "PAGE_CE_REFRESH_CYCLE",
       .units = cycle_units,
       .var = 24,
       .unit = "h",
};

(where .unit could either be seconds, minutes, hours or days - not sure what would work best )

Under normal circumstances, you’re totally right. But here’s the edge case I’m thinking about: If I set a crazy huge value like 213503982334062d, here’s what happens in the code logic. First, it doesn’t hit the maximum int limit, so no truncation. Then when converting it to the default unit, boom—(213503982334062d to 18446744073662956800s )suddenly the converted value exceeds the max limit. But here’s the catch: If I only use either 'env' or 'unit' to store the working value, I completely lose track of the original value the user actually input. That means when I need to show error messages later, I can’t tell the user 'Hey your original 213503982334062d was invalid'—I’d only have the messed-up converted value. So here’s the thing—we can’t have both accurate value processing AND accurate error reporting if we only use one storage variable. That’s why I ended up keeping both 'env' (for the original user input) and 'val' (for the working value). This way we handle the math correctly while still being able to point to exactly what the user typed when things go wrong.

The issue is because we're declaring things like:

struct isolation {
	char			*name;
	char			*env;
	const struct config	*units;
	unsigned long	val;
	bool			overflow;
	char			*unit;
};

static struct isolation cycle = {
	.name = "PAGE_CE_REFRESH_CYCLE",
	.units = cycle_units,
	.env = "24h",
	.unit = "h",
};

And then let user change what's there. I'm not a big fan of having a non-const default structure, as this makes it harder to identify changes. Anyway, the imutable data including the default value should be const. Also, the name seems to be const as well.

IMO, the best solution is to split const from non-const data, e.g.:

struct isolation_props {
	char		*name;
	unsigned long	default;
	char		*default_unit;
	struct config	*units;
}

struct isolation {
	const struct isolation_props props;

	unsigned long	value;
	const char	*unit;
};

static struct isolation cycle = {
	.props = {
		.name = "PAGE_CE_REFRESH_CYCLE",
		.units = cycle_units,
		.default = 24 * 60 * 60 /* seconds */,
	}
};

And ensure that value and unit will be properly set based on the default values and the user input.

Please notice that I removed overflow, as, on overflow, the logic should set to default.

Thanks for the suggestion! Just to clarify one thing: if we detect a value overflow during unit conversion, we currently fall back to the default "24h". But since "24h" itself needs to be converted to seconds (→ 86400s), wouldn't it be simpler to initialize val directly with 86400 upfront? This way, we avoid redundant conversions when handling the default case.