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/.
Pau Espin Pedrol gerrit-no-reply at lists.osmocom.org
Patch Set 1: Code-Review-1
(4 comments)
Fix the small bits and it's fine for me. Once it's merged we can apply some other bits in my patch which are also useful (documentation., extra check for osmux frame type, etc.).
Please before doing so, also rebase on top of latest master, otherwise there are gerrit verification issues due to some late changes when verifying some stuff during build time.
https://gerrit.osmocom.org/#/c/3825/1//COMMIT_MSG
Commit Message:
Line 31: As in snprintf(), caller should not assume the buffer is nul-terminated.
Why
not? Reading at man snprintf my impression is the opposite:
"""
RETURN VALUE
Upon successful return, these functions return the number of
characters printed (excluding the null byte used to end output to
strings).
The functions snprintf() and vsnprintf() do not write more than
size bytes (including the terminating null byte ('\0')). If the
output was truncated due to this limit, then the return value is the
number of characters (excluding the terminating null byte) which would
have been written to the final string if enough space had been
available. Thus, a return value of size or more means that the output
was truncated. (See also below under NOTES.)
If an output error is encountered, a negative value is returned.
"""
So, as long as siz>=1, it for sure should write the NULL character.
Other possibilities I can think of is the case in which snprintf()
returns -1, where I guess the output buffer should not be used anyway
because it's unexpected what's in there. And in the case size==0, I'd
bet it either:
- Returns -1 (case above)
- Returns number of bytes required for the input buffer (and you don't
care if it wrote null byte because there's actually no input buffer and
if it's not intended to have size=0 you already have bigger problems
somewhere else.
It's true though that as we are masking the error in snprintf, what comes out of osmux_snprintf may not be null terminated.
https://gerrit.osmocom.org/#/c/3825/1/tests/osmux/osmux_test.c
File tests/osmux/osmux_test.c:
Line 87: buf[ret - 1] = '\0';
As stated in the comment I did in the commit description, I think we should always ensure a null terminated buf is returned (given than size>0 of course). I'd move this line inside osmo_rtp_snprintf.
Line 113: buf[ret - 1] = '\0';
Same here for osmux_snprintf.
Line 208: buf[ret - 1] = '\0';
same
--
To view, visit https://gerrit.osmocom.org/3825
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b5d6ec57a02f57c23b1ae86dbd894bad28ea797
Gerrit-PatchSet: 1
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Owner: Pablo Neira Ayuso <pablo at gnumonks.org>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-HasComments: Yes