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 21:53 +0200, Pablo Neira Ayuso wrote:
> 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.

v2 of the patchset is supposed to do that. wdyt?


Thanks
Thomas