Fix numeric config boundry check#14286
Conversation
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) |
|
New Issues (14)Checkmarx found the following issues in this Pull Request
Fixed Issues (5)Great job! The following issues were fixed in this Pull Request
|
src/config.c
Outdated
| /* Boundary check for unsigned types | ||
| * The upper/lower bound check should suffice but for UINT type if | ||
| * negative value is passed that certainly means an error. This check | ||
| * is only here for the module config API - see moduleSetNumericConfig. | ||
| * If the numeric config was not set through a module, the codepath | ||
| * would be through numericConfigSet, which deals with wrong values | ||
| * via numericParseString. */ | ||
| if (config->data.numeric.numeric_type == NUMERIC_TYPE_UINT && ll < 0) { |
There was a problem hiding this comment.
feel that the code is not reabable. Maybe we can directly move this check into moduleSetNumericConfig and explain that it is to avoid introducing break change.
There was a problem hiding this comment.
Ok makes sense.
sundb
added
the
release-notes
|
@oranagra Do you think adding a new module api With RM_ConfigSetUnsignedConfig the parameter would be unsigned long long. |
|
looking at all the config related interfaces we have, none of them use unsigned, so either we add it across the board, or suffice with what we have. |
I guess if we didn't need unsigned up until now we are safe to move on without them? Only thing I'm worried, I rmbr talking that maybe it's not a good idea to revert the breaking change since we already introduced a breaking change and unbreaking it would be a second breaking change. How do we feel about that? |
i wouldn't argue that we didn't need them, maybe some modules did, maybe they didn't even realize that.
in many times there's a gap between the docs and the code, it could be because of a misunderstanding, or because we often care less about docs, but either way, people use what works, even if the doc says otherwise, and if there's a conflict we'll often decide to fix the doc.
if the breakage is fairly recent, i think it's better to unbreak it, i think it's likely that people didn't upgrade and didn't notice it. |
so you suggest we put a boundary check for unsigned inside |
4 participants
Revert a breaking change introduced in #14051 described in this comment #14051 (comment)
The non-negative check inside
checkNumericBoundarieswas ignoring that passing a big unsigned long long value (> 2^63-1) will be passed as negative value and will never reach the lower/upper boundary check.The check is removed, reverting the breaking change.
Note that this allows the ambiguity of passing a negative value for unsigned long long configs as moduleSetNumericConfig has
long longparam for the value. The negative value would be implicitly converted to unsigned long long. Further discussion on how to resolve the ambiguity would be needed.