Fix src/check_pack.c #250
Fix src/check_pack.c #250
Conversation
* Check before calling the function that input value fits
in 32 bits.
Attn. This refactoring is done without changing the interface
(check_pack.h), including the messages. To do this, a lot of
paranoid programming is done to ensure values fit the types.
Signed-off-by: Mikko Johannes Koivunalho <[email protected]>
New signatures:
static size_t pack_ctx(char **buf, CtxMsg * cmsg)
static size_t pack_loc(char **buf, LocMsg * lmsg)
static size_t pack_fail(char **buf, FailMsg * fmsg)
static size_t pack_duration(char **buf, DurationMsg * fmsg)
* Interface functions (check_pack.h) pack() and upack() return int
so return values are checked if they fit int type.
Attn. This refactoring is done without changing the interface
(check_pack.h), including the messages. To do this, a lot of
paranoid programming is done to ensure values fit the types.
Signed-off-by: Mikko Johannes Koivunalho <[email protected]>
* Create macro CK_DIAGNOSTIC_PUSH_IGNORE and CK_DIAGNOSTIC_POP
* Ignore two cases of -Wbad-function-cast because
temporary variables would just be completely useless
Signed-off-by: Mikko Johannes Koivunalho <[email protected]>
I wanted to have this also but not possible (explain below):
if(n > SSIZE_MAX)
eprintf("get_result returned value (%z) too big for ssize_t, max allowed %d\n",
* Do not use SSIZE_MAX (with ssize_t) because it is POSIX.
This creates additional and confusing casts, unfortunately.
Signed-off-by: Mikko Johannes Koivunalho <[email protected]>
|
|
||
|
|
||
| static void pack_int(char **buf, int val); | ||
| static int upack_int(char **buf); | ||
| static void pack_int(char **buf, ck_uint32 val); |
There was a problem hiding this comment.
I'm wondering what the value of having the ck_uint32 typedef is, rather than just using uint32_t directly. I'm not sure it buys anything. Same for UINT32_MAX vs CK_UINT32_MAX. What do you think?
There was a problem hiding this comment.
Well, ck_uint32 was there when I came. 😁
I added CK_UINT32_MAX for the sake of consistence.
ck_uint32 is only used in check_pack.c and only internally, not in the header. It would not be a big thing to refactor it away.
src/check_pack.c
Outdated
| @@ -196,7 +202,11 @@ static char *upack_str(char **buf) | |||
| char *val; | |||
| int strsz; | |||
|
|
|||
| strsz = upack_int(buf); | |||
| ck_uint32 str_sz = upack_int(buf); | |||
| if(str_sz > INT_MAX) | |||
There was a problem hiding this comment.
The check for the pack and unpack are not the same. A check for UINT32_MAX is done for the pack, but a check for INT_MAX is done for the unpack. Should the check be the same, and use the smaller of UINT32_MAX or INT_MAX?
There was a problem hiding this comment.
I see the point looking at some of the other commits that do the same thing. It is not about catching an issue when the value is first packed, but doing a sanity check before the value is used. That should be fine. It catches an issue before the data is used improperly.
There was a problem hiding this comment.
Yes. It checks that the value is not too big. In fact, I am unhappy about writing a ck_uint32 to an int, because int's size could be smaller (and system dependent). In fact, if int is 32 bit, it is certainly too small because of the unsigned to signed conversion.
But DurationMsg is part of the API (the public header). If that were changed, it would remove the need for many checks.
|
Note to self: Travis-CI deprecated how it reports status back on builds. A build was triggered and passed: link. |
Some refactoring done in
src/check_pack.c. The interfacesrc/check_pack.his unchanged. Because of this, I have added several casts. I have tried to work around the problems of signed/unsigned arithmetic.