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

[Thomas Haller <thaller@redhat.com>]

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?


Thomas