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 <[email protected]> Gerrit-Reviewer: Harald Welte <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-HasComments: Yes
