-
Notifications
You must be signed in to change notification settings - Fork 79
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix the bug that config->env
is greater than ulong_max
when units->val =1
#130
base: master
Are you sure you want to change the base?
Conversation
config->env
is greater than ulong_max
when units…config->env
is greater than ulong_max
when units->val =1
if (errno == ERANGE || *endptr != '\0') | ||
config->overflow = true; | ||
} | ||
unit_matched = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic seems weird to me. First of all, units is an array. It may potentially have units->name == NULL as the first element, indicating the end of such array, in which case units->val should not be used.
Also, I tried to understand why units->val==1 so special. I mean, why other values can't overflow?
Looking at the check code:
/* if env value string is greater than ulong_max, truncate the last digit */
sscanf(config->env, "%lu", &value);
for (units = config->units; units->name; units++) {
if (!strcasecmp(config->unit, units->name))
unit_matched = 1;
if (unit_matched) {
tmp = value;
value *= units->val;
if (tmp != 0 && value / tmp != units->val)
config->overflow = true;
}
}
The idea of the above check were exactly to catch ERANGE issues. If it is broken, or should instead use strtoul to catch overflows, please fix it or replace the check by a better logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if units->val==1
and PAGE_CE_THRESHOLD
or PAGE_CE_REFRESH_CYCLE
is greater than ulong_max.
e.g.
PAGE_CE_THRESHOLD=9999999999999999999999999999999999999999999999999
PAGE_CE_REFRESH_CYCLE=333333999999999999999999999999999999999s
/* if env value string is greater than ulong_max, truncate the last digit */
sscanf(config->env, "%lu", &value);
for (units = config->units; units->name; units++) {
if (!strcasecmp(config->unit, units->name))
unit_matched = 1;
if (unit_matched) {
tmp = value;
value *= units->val;
if (tmp != 0 && value / tmp != units->val)
config->overflow = true;
}
}
then sscanf(config->env, "%lu", &value);
value is 18446744073709551615(2^64-1)
value *= units->val;
// value is 2^64-1, because units->val == 1
if (tmp != 0 && value / tmp != units->val)
and then that condition is not met. because (value / tmp == 1)
but units->val
is not 1, e.g. 1000,
then sscanf(config->env, "%lu", &value);
value is 18446744073709551615(2^64-1)
value *= units->val;
// value is 2^64-1, Even though value is multiplied by 1000, the maximum value is still 2^64-1
if (tmp != 0 && value / tmp != units->val)
and then that condition is met. because (value / tmp == 1, not 1000)
This is just showing overflow, it doesn't actually seem to be overflowing because the maximum value can only be 2^64 - 1 |
PAGE_CE_THRESHOLD=9999999999999999999999999999999999999999999999999k
|
when
PAGE_CE_THRESHOLD
is greater than ulong_max, it can be set