Re: [PATCH nft 5/8] src: rework SNPRINTF_BUFFER_SIZE() and avoid "-Wunused-but-set-variable"

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Mon, Aug 28, 2023 at 06:45:00PM +0200, Thomas Haller wrote:
> On Mon, 2023-08-28 at 18:04 +0200, Pablo Neira Ayuso wrote:
> > On Mon, Aug 28, 2023 at 05:49:53PM +0200, Thomas Haller wrote:
> > > On Mon, 2023-08-28 at 17:13 +0200, Pablo Neira Ayuso wrote:
> > 
> > > SNPRINTF_BUFFER_SIZE() rejects truncation of the string by
> > > asserting
> > > against it. That behavior is part of the API of that function.
> > > Error
> > > checking after an assert seems unnecessary.
> > > 
> > > The check "if (len == NF_LOG_PREFIXLEN)" seems wrong anyway. After
> > > truncation, "len" would be zero. The code previously checked
> > > whether
> > > nothing was appended, but the error string didn't match that
> > > situation.
> > > 
> > > Maybe SNPRINTF_BUFFER_SIZE() should not assert against truncation?
> > 
> > IIRC, the goal for this function was to handle snprintf() and all its
> > corner cases. If there is no need for it or a better way to do this,
> > this is welcome.
> > 
> 
> I think the macro is sensible (at least, after some cleanup).
> 
> It makes a choice, that the caller must ensure a priori that the buffer
> is long enough (by asserting).
> 
> By looking at the callers, it's not clear to me, whether the callers
> can always ensure that.  For meta_key_parse(), it seems the maximum
> string is limited by meta_templates. But for stmt_evaluate_log_prefix()
> it may be possible to craft user-input that triggers the assertion,
> isn't it?
> 
> Maybe the macro and the callers should anticipate and handle
> truncation?

This should not silently truncate strings, instead bail out to user in
case the string is too long.

#define NF_LOG_PREFIXLEN       128

Maximum length as specified by uapi/linux/netfilter/nf_log.h

Currently in userspace, if I specify a string longest than that, I can
see ASAN complains on incorrect memory management from userspace
(without your patchset), so this code is currently broken (by me
mostly likely since I was the last one to touch those bits).

While at this, it would be good to fix it and add a test to cover
maximum prefix length.

Regarding meta_key_parse(), these days the preferred approach is to
use start conditions in flex. This function should go away, it was an
early attempt to reduce tokens by using STRING from bison, which
turned out to be flawed.

It is not worth to fix meta_key_parse(), flex start conditions and
bison parser should be used.

This macro is hiding behind a bit of technical debt.