Patch Set 4: (4 comments)
https://gerrit.osmocom.org/#/c/5438/4/src/ctrl/control_cmd.c File src/ctrl/control_cmd.c: Line 284: static bool id_str_valid(const char *str) > Are we absolutely sure that all current and future users of this function a yes, the strtok actually places a '\0' after each token (and overwrites the input string I think) Line 296: struct ctrl_cmd *ctrl_cmd_parse2(void *ctx, struct msgb *msg) > We take msgb as input so we know the length of data to process. This means the msgb length is the entire command's length, the id string is only the second token, separated out by strtok. And furthermore, the start of this function actually adds a '\0' to the end of the msgb to be sure that the string is nul terminated... Remaining issue could be that the msgb already contains a '\0' halfway thru and there might be more data following which would be ignored -- not related to this patch. Line 331: tmp = strtok_r(NULL, " ", &saveptr); here the id string gets separated and terminated Line 341: if (!id_str_valid(tmp)) { all i do here is validate the resulting string which is guaranteed to be terminated -- To view, visit https://gerrit.osmocom.org/5438 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I96a9b6b6a3a5e0b80513aa9eaa727ae8c9c7d7a1 Gerrit-PatchSet: 4 Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Owner: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: Harald Welte <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Max <[email protected]> Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-HasComments: Yes
