After reading the bug report #3593, it prompted me to perform a review of huawei-ups2000 driver again as its original author. Unfortunately, I found a serious regression introduced during the "fightwarn" campaign in e9f02e2 that completely broke bypass.start, shutdown.return, shutdown.reboot, and shutdown.reboot.graceful.
Unlike the cited bug, this bug affects all NUT versions, all UPS hardware, and all firmware revisions.
Any attempt to use them will fail, with an error logged to syslog, for example:
huawei-ups2000: instcmd: command [bypass.start] reg1 is negative
huawei-ups2000: instcmd: command [shutdown.return] reg1 is negative
huawei-ups2000: instcmd: command [shutdown.reboot] reg1 is negative
huawei-ups2000: instcmd: command [shutdown.reboot.graceful] reg1 is negative
Because the regression was introduced shortly after the initial driver was merged into the upstream, as a result, instant commands do no work in any version of NUT, with the exception of the earliest development build. This problem was not discovered during my test for the same reason: it's a regression that occurred after the Pull Request was merged.
Contributing Factors and Context
This regression was introduced due to a combination of two factors:
-
During the development, I mistakenly believed that GCC -Wextra warnings were turned on, but in fact they were turned off! I didn't notice it. As a result, many unsafe operations such as signness confusions existed in my original Pull Request.
-
The merge of the Pull Request coincided with NUT's 2.8.0 refactoring, during a project refactor campaign called "fightwarn" with the goal of suppressing all compiler warnings. Unfortunately, because it was rushed, the fixes themselves are often "brute-force" in nature. If the compiler says there's an integer overflow, the fix was to cast all possible integer values instead of fix the location of the overflow. If the compiler says there's a buffer overflow, the fix (even to a compiler false-positive) was to add range-checked in all locations where the value is used, rather than fixing the logic.
For example, one commit was introduced in huawei-ups2000 to suppress an integer overflow warning with three typecasts, when one casting was necessary.
crc16_recv = (uint16_t)((uint16_t)(ident_response_end[0]) << 8) | (uint16_t)(ident_response_end[1]);
For another example, one commit introduced a brute-force range check to a crc16() function, this was suboptimal since changing crc16() to accept size_t instead of uint16_t would be safer.
if (ident_response_len < IDENT_RESPONSE_CRC_LEN
|| (((uintmax_t)(ident_response_len) - IDENT_RESPONSE_CRC_LEN) > UINT16_MAX)
) {
fatalx(EXIT_FAILURE, "response header shorter than CRC "
"or longer than UINT16_MAX!");
}
crc16_calc = crc16(ident_response, ident_response_len - IDENT_RESPONSE_CRC_LEN);I later submitted a Pull Request, which partially improved these brute-force changes.
However, I did not notice all changes, and missed the critical instant command regression.
Root Cause Analysis
In huawei-ups2000, all instant commands are driven by a lookup table to consolidate the logic to a central dispatcher.
/*
* A lookup table of all instant commands "cmd" and their
* corresponding registers "reg". For each instant command,
* it's handled by...
*
* 1. One register write, by writing "val1" to "reg1", the
* simplest case.
*
* 2. Two register writes, by writing "val1" to "reg1", and
* writing "val2" to "reg2". One after another.
*
* 3. Calling "*handler_func" and passing "reg1". This is
* used to handle commands that needs additional processing.
* If "reg1" is not necessary or unsuitable, "-1" is used.
*/
#define REG_NULL -1, -1
#define FUNC_NULL NULL
static struct ups2000_cmd_t {
const char *cmd;
const int16_t reg1, val1, reg2, val2;
int (*const handler_func)(const uint16_t);
} ups2000_cmd[] =
{
{ "test.battery.start.quick", 2028, 1, REG_NULL, FUNC_NULL },
{ "test.battery.start.deep", 2021, 1, REG_NULL, FUNC_NULL },
{ "test.battery.stop", 2023, 1, REG_NULL, FUNC_NULL },
{ "beeper.enable", 1046, 0, REG_NULL, FUNC_NULL },
{ "beeper.disable", 1046, 1, REG_NULL, FUNC_NULL },
{ "load.off", 1045, 0, 1030, 1, FUNC_NULL },
{ "bypass.stop", 1029, 1, 1045, 0, FUNC_NULL },
{ "load.on", 1029, -1, REG_NULL, ups2000_instcmd_load_on },
{ "bypass.start", REG_NULL, REG_NULL, ups2000_instcmd_bypass_start },
{ "beeper.toggle", 1046, -1, REG_NULL, ups2000_instcmd_beeper_toggle },
{ "shutdown.stayoff", 1049, -1, REG_NULL, ups2000_instcmd_shutdown_stayoff },
{ "shutdown.return", REG_NULL, REG_NULL, ups2000_instcmd_shutdown_return },
{ "shutdown.reboot", REG_NULL, REG_NULL, ups2000_instcmd_shutdown_reboot },
{ "shutdown.reboot.graceful", REG_NULL, REG_NULL, ups2000_instcmd_shutdown_reboot_graceful },
{ NULL, -1, -1, -1, -1, NULL },
};The comment explains three cases, and it's important to note Case 3 here:
- Calling
*handler_func and passing reg1. This is used to handle commands that needs additional processing. If reg1 is not necessary or unsuitable, -1 is used.
In other words, not all *handler_func actually reads reg1, it's an optional argument despite that the function signature always includes it. Because declaring the function as a variadic function or a (void *) function would be over-engineering, a simple unsigned 16-bit value is used.
When the *handler_func doesn't use reg1, reg1 is silently ignored. But, during the fightwarn refactor campaign, to make the code "safe". The compiler generates a warning because it saw that the signed -1 is sometimes passed to *handler_func as an unsigned value, becoming 65535. As a result, an incorrect range check was added to the function, which refuses to invoke *handler_func if reg1 is negative.
--- a/drivers/huawei-ups2000.c
+++ b/drivers/huawei-ups2000.c
@@ -1495,13 +1495,18 @@ static int instcmd(const char *cmd, const char *extra)
if (cmd_action->handler_func) {
/* handled by a function */
- status = cmd_action->handler_func(cmd_action->reg1);
+ if (cmd_action->reg1 < 0) {
+ upslogx(LOG_WARNING, "instcmd: command [%s] reg1 is negative", cmd);
+ return STAT_INSTCMD_UNKNOWN;
+ } else {
+ status = cmd_action->handler_func((uint16_t)cmd_action->reg1);
+ }
}Because we have four instant commands that don't take any registers:
{ "bypass.start", REG_NULL, REG_NULL, ups2000_instcmd_bypass_start },
{ "shutdown.return", REG_NULL, REG_NULL, ups2000_instcmd_shutdown_return },
{ "shutdown.reboot", REG_NULL, REG_NULL, ups2000_instcmd_shutdown_reboot },
{ "shutdown.reboot.graceful", REG_NULL, REG_NULL, ups2000_instcmd_shutdown_reboot_graceful },These calls are now always rejected.
The correct fix was to fix the logic, not to suppress the symptom: make reg1 an unsigned value in the LUT.
I'll submit a patch soon to fix it.
Comments
This regressions shows the undesirable effects of:
-
Suppressing compiler warnings for the sake of the suppressing warnings.
-
Not notifying the respective maintainers to review changes in drivers in which they're responsible.
In some projects, a "code ownership" model is used: no change is made to any subsystems until the subsystem maintainer approve it (I believe the early days of NetBSD did that). In some projects, everyone is free to change whatever subsystems they see fit (which is the Linux kernel's model). I believe NUT would be better if a middle ground position is taken: a mechanism to notify driver maintainers to review changes would reduce these incidents. To prevent unresponsive maintainers from slowing down the project, the review can be made optional, a single GitHub CC may be sufficient.
After reading the bug report #3593, it prompted me to perform a review of
huawei-ups2000driver again as its original author. Unfortunately, I found a serious regression introduced during the "fightwarn" campaign in e9f02e2 that completely brokebypass.start,shutdown.return,shutdown.reboot, andshutdown.reboot.graceful.Unlike the cited bug, this bug affects all NUT versions, all UPS hardware, and all firmware revisions.
Any attempt to use them will fail, with an error logged to
syslog, for example:Because the regression was introduced shortly after the initial driver was merged into the upstream, as a result, instant commands do no work in any version of NUT, with the exception of the earliest development build. This problem was not discovered during my test for the same reason: it's a regression that occurred after the Pull Request was merged.
Contributing Factors and Context
This regression was introduced due to a combination of two factors:
During the development, I mistakenly believed that GCC
-Wextrawarnings were turned on, but in fact they were turned off! I didn't notice it. As a result, many unsafe operations such as signness confusions existed in my original Pull Request.The merge of the Pull Request coincided with NUT's 2.8.0 refactoring, during a project refactor campaign called "fightwarn" with the goal of suppressing all compiler warnings. Unfortunately, because it was rushed, the fixes themselves are often "brute-force" in nature. If the compiler says there's an integer overflow, the fix was to cast all possible integer values instead of fix the location of the overflow. If the compiler says there's a buffer overflow, the fix (even to a compiler false-positive) was to add range-checked in all locations where the value is used, rather than fixing the logic.
For example, one commit was introduced in
huawei-ups2000to suppress an integer overflow warning with three typecasts, when one casting was necessary.For another example, one commit introduced a brute-force range check to a
crc16()function, this was suboptimal since changingcrc16()to acceptsize_tinstead ofuint16_twould be safer.I later submitted a Pull Request, which partially improved these brute-force changes.
However, I did not notice all changes, and missed the critical instant command regression.
Root Cause Analysis
In
huawei-ups2000, all instant commands are driven by a lookup table to consolidate the logic to a central dispatcher.The comment explains three cases, and it's important to note Case 3 here:
In other words, not all
*handler_funcactually readsreg1, it's an optional argument despite that the function signature always includes it. Because declaring the function as a variadic function or a(void *)function would be over-engineering, a simple unsigned 16-bit value is used.When the
*handler_funcdoesn't usereg1,reg1is silently ignored. But, during the fightwarn refactor campaign, to make the code "safe". The compiler generates a warning because it saw that the signed-1is sometimes passed to*handler_funcas an unsigned value, becoming 65535. As a result, an incorrect range check was added to the function, which refuses to invoke*handler_funcifreg1is negative.Because we have four instant commands that don't take any registers:
{ "bypass.start", REG_NULL, REG_NULL, ups2000_instcmd_bypass_start }, { "shutdown.return", REG_NULL, REG_NULL, ups2000_instcmd_shutdown_return }, { "shutdown.reboot", REG_NULL, REG_NULL, ups2000_instcmd_shutdown_reboot }, { "shutdown.reboot.graceful", REG_NULL, REG_NULL, ups2000_instcmd_shutdown_reboot_graceful },These calls are now always rejected.
The correct fix was to fix the logic, not to suppress the symptom: make
reg1an unsigned value in the LUT.I'll submit a patch soon to fix it.
Comments
This regressions shows the undesirable effects of:
Suppressing compiler warnings for the sake of the suppressing warnings.
Not notifying the respective maintainers to review changes in drivers in which they're responsible.
In some projects, a "code ownership" model is used: no change is made to any subsystems until the subsystem maintainer approve it (I believe the early days of NetBSD did that). In some projects, everyone is free to change whatever subsystems they see fit (which is the Linux kernel's model). I believe NUT would be better if a middle ground position is taken: a mechanism to notify driver maintainers to review changes would reduce these incidents. To prevent unresponsive maintainers from slowing down the project, the review can be made optional, a single GitHub CC may be sufficient.