This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
Harald Welte gerrit-no-reply at lists.osmocom.org
Patch Set 1:
(5 comments)
https://gerrit.osmocom.org/#/c/4146/1/include/osmocom/mgcp_client/mgcp_client.h
File include/osmocom/mgcp_client/mgcp_client.h:
PS1, Line 49: MSG_T
msg_t typically referst to "message type", which it isn't. What about MGCP_MSG_PRES_... ? or MGCP_MGS_PAR_... ?
https://gerrit.osmocom.org/#/c/4146/1/src/libosmo-mgcp-client/mgcp_client.c
File src/libosmo-mgcp-client/mgcp_client.c:
Line 572: /* Deprecated, please use mgcp_msg_gen() instead */
We do have a way to annotate our functions as deprecated that will lead to compiler warnings when code uses them. See OSMO_DEPRECATED
Line 631: static char compose[4096 - 128];
where do these magic numbers come from? how are we sure we don't overflow them?
Line 639: switch (mgcp_msg->verb) {
this could be a 'const' table in the code rather than an open-coded select in the code, but don't bother changing it, just maybe as an idea you run into this kind of problem next time.
Line 671: sprintf(compose + strlen(compose), " %s", mgcp_msg->endpoint);
so we have a long series of "sprintf(compose + strlen(compose)" statements here, and in the end we generate a msgb from it. This means we have dozens of calls to strlen(), we have a buffer on the stack and we don't really check whether we overflow that buffer or not.
What about instead doing this in the msgb itself? I.e. first allocate the msgb, and then have msgb_append_str() or msgb_append_sprintf() or the like. That function then will use msgb_put() which in turn ensures sufficient tailroom. The msgb will also keep track of what was the last byte written (offset) so we avoid all the strlen() calls.
In summary, we get safer code and possibly even faster code (performance is not key here, the safety aspect is what I care).
Alternatively, there's talloc_asprintf_append() which is a safe method of appending a format-printed string to an existing string buffer. However, that's of course on the heap and each call might likely re-allocate the underliyng buffer, so we'd have a dozen or so dynamic memory allocations in this function. Probably not as fast, but also safe.
--
To view, visit https://gerrit.osmocom.org/4146
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I29c5e2fb972896faeb771ba040f015592487fcbe
Gerrit-PatchSet: 1
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-HasComments: Yes