Hi Florian, On Mon, Sep 24, 2018 at 11:11:59AM +0200, Florian Westphal wrote: > Phil Sutter <phil@xxxxxx> wrote: > > Make sure destination buffers are NULL-terminated by replacing strcpy() > > with strncat() (if destination is guaranteed to be zeroed) or explicitly > > set last byte in buffer to zero. > > I'm sorry, but i don't like this at all. > > > - strcpy(cs->target->t->u.user.name, cs->jumpto); > > + strncat(cs->target->t->u.user.name, cs->jumpto, > > + XT_EXTENSION_MAXNAMELEN - 1); > > This reads "append this to user.name", even though this is > supposed to copy. Yes, admittedly this is a bit of strncat() misuse simply because it always appends the terminating 0. > I realize user.name is 0-terminated, but this is yet one > more thing one "has to know". Well, cs->target->t was allocated using calloc() a few lines above so the whole buffer is zeroed. Otherwise this might really do the wrong thing. But yes, it's something one might overlook while refactoring these match creators (which occur in nearly identical form all over the code). > So, this should either be > strcpy (no change) > strncpy + setting last element to 0, > snprintf() > > I think use of *cat() should be limited to cases where > we want to append, not to work around warnings. I don't like the first option since it requires to make sure input really can't be too long. Whenever I do the second one (which I actually did at first), I'm tempted to write a macro to combine the two actions. I would like to do one of: - use snprintf(), - use strlcpy() from libbsd or - introduce a poor-man's strlcpy() macro/function. What would you prefer? Leave everything as-is, one of the above or something completely different? :) Thanks, Phil
