libosmo-netif[master]: osmux: Re-write osmux_snprintf

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
Fri Aug 25 14:42:55 UTC 2017


Patch Set 1:

> @Pau: You're not the only one getting confused with it, snprintf()
 > is a mess, it's hard to deal with it, hence this macro that retains
 > semantics that aims to simplify things... if you find any better,
 > let me know I'd be happy to reuse it in my code moving forward :-).
 > 
 > @Holger: Either way, I don't mind if you don't need the snprintf
 > semantics, this is a _snprintf() function after all, just explaning
 > here. So the intention at least that I can remember was to keep in
 > sync with how snprintf() works. Anyway, feel free to simplify this.
 > 

Even if the intention was to follow snprintf, as far as I can tell it doesn't seem to be actually working correctly. Implementation previous to this patch returns offset in all functions. offset var is always calculated using this part of the macro:
	if (ret > len)					\
		ret = len;				\
	offset += ret;					\

Which means offset is never going to be bigger than the "size" originally passed as parameter. That's correct as offset marks internally next position to write in the buffer, but should not be the var being returned as it's always <= size even if size of buffer is smaller than required size.

In exchange, size (from the variable) should be initialized to 0 at the start of each function after assigning len = size, then return size in each function. This could be called "count" instead of "size" to avoid confusion with "size in parameter. Actually, I don't really understand what's the usage of the "size" variable as used currently, which starts having size of the buffer (used or not) and keeps growing.

On top of that, I have the feeling the previous implementation doesn't handle the case where snprintf returns negative value (error). So even after all those modifications, we still need to add extra logic to handle that. In nftables code it seems to be handled by calling abort(), which doesn't seem like a perfect approach in any case. It's not nice having libraries calling abort().

 > Anyway, I just wanted to make sure this patch was really fixing up
 > the real issue. I understand you observe a crash, but not clear to
 > me why the patch is fixing it.
 > 
 > Cheers.


I try to fix all this corner cases explained above in this patch. I started looking again today at following a more conservative approach, like fixing step by step the current code, but as far as I can tell doesn't make sense, as there's a lot of stuff which needs changes.

So, if there's no more objections to this patch, during following days I'll push a new version with the description fixes as advised by Holger, and we can then merge it.

-- 
To view, visit https://gerrit.osmocom.org/3537
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I695771d099833842db37a415b636035d17f1bba7
Gerrit-PatchSet: 1
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Owner: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-Reviewer: Holger Freyther <holger at freyther.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Pablo Neira Ayuso <pablo at gnumonks.org>
Gerrit-Reviewer: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-HasComments: No



More information about the gerrit-log mailing list