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 17:13 +0200, Pablo Neira Ayuso wrote:
> On Mon, Aug 28, 2023 at 04:43:55PM +0200, Thomas Haller wrote:
> > SNPRINTF_BUFFER_SIZE() causes a warning with clang.
> > 
> >     evaluate.c:4134:9: error: variable 'size' set but not used [-
> > Werror,-Wunused-but-set-variable]
> >             size_t size = 0;
> >                    ^
> > 
> >     meta.c:1006:9: error: variable 'size' set but not used [-
> > Werror,-Wunused-but-set-variable]
> >             size_t size;
> >                    ^
> > 
> > Fix that, and rework SNPRINTF_BUFFER_SIZE().
> > 
> > - before and now, the macro asserts against truncation. Remove
> > error
> >   handling related to truncation in the callers.
> > 
> > - wrap the macro in "do { ... } while(0)" to make it more
> >   function-like.
> > 
> > - evaluate macro arguments exactly once, to make it more function-
> > like.
> > 
> > - take pointers to the arguments that are being modified.
> > 
> > - use assert() instead of abort().
> > 
> > - use size_t type for arguments related to the buffer size.
> > 
> > - drop "size". It was unused, and, unless the string was truncated,
> >   it was identical to "offset".
> > 
> > - "offset" previously was incremented before checking for
> > truncation.
> >   So it would point somewhere past the buffer. This behavior is not
> >   useful, because we assert against truncation. Now, in case of
> >   truncation, "len" will be zero and "offset" will be the original
> > "len"
> >   (that is, point after the buffer and one byte after the
> > terminating NUL).
> > 
> > Signed-off-by: Thomas Haller <thaller@redhat.com>
> > ---
> >  include/utils.h | 32 +++++++++++++++++++++++---------
> >  src/evaluate.c  | 11 +++++------
> >  src/meta.c      | 10 +++++-----
> >  3 files changed, 33 insertions(+), 20 deletions(-)
> > 
> > diff --git a/include/utils.h b/include/utils.h
> > index cee1e5c1e8ae..873147fb54ec 100644
> > --- a/include/utils.h
> > +++ b/include/utils.h
> > @@ -72,15 +72,29 @@
> >  #define max(_x, _y) ({                         \
> >         _x > _y ? _x : _y; })
> >  
> > -#define SNPRINTF_BUFFER_SIZE(ret, size, len, offset)   \
> > -       if (ret < 0)                                    \
> > -               abort();                                \
> > -       offset += ret;                                  \
> > -       assert(ret < len);                              \
> > -       if (ret > len)                                  \
> > -               ret = len;                              \
> > -       size += ret;                                    \
> > -       len -= ret;
> > +#define SNPRINTF_BUFFER_SIZE(ret, len, offset)                 \
> > +       do { \
> > +               const int _ret = (ret);                         \
> > +               size_t *const _len = (len);                     \
> > +               size_t *const _offset = (offset);               \
> > +               size_t _ret2;                                   \
> > +                                                               \
> > +               assert(_ret >= 0);                              \
> > +                                                               \
> > +               if ((size_t) _ret >= *_len) {                   \
> > +                       /* Fail an assertion on truncation.
> > +                        *
> > +                        * Anyway, we would set "len" to zero and
> > "offset" one
> > +                        * after the buffer size (past the
> > terminating NUL
> > +                        * byte). */                            \
> > +                       assert((size_t) _ret < *_len);          \
> > +                       _ret2 = *_len;                          \
> > +               } else                                          \
> > +                       _ret2 = (size_t) _ret;                  \
> > +                                                               \
> > +               *_offset += _ret2;                              \
> > +               *_len -= _ret2;                                 \
> > +       } while (0)
> 
> This macro is something I made myself, which I am particularly not
> proud of it, but it getting slightly more complicated.

IMO it just got simpler. E.g. it is now function-like; you can easier
see which arguments are modified (we take a pointer to them); one
argument got dropped.

The remaining relevant parts (assertions and truncation check aside) is
literally

   offset += ret;
   len -= ret;


> 
> Probably it time to turn this into a real function?

as it now behaves function-like, it can be easily converted to an
inline function. Only difference is that upon assertion failure, we no
longer see the location of the caller. It does not seem an improvement.


> 
> >  #define MSEC_PER_SEC   1000L
> >  
> > diff --git a/src/evaluate.c b/src/evaluate.c
> > index 1ae2ef0de10c..f8cd7b7afda3 100644
> > --- a/src/evaluate.c
> > +++ b/src/evaluate.c
> > @@ -4129,14 +4129,16 @@ static int stmt_evaluate_queue(struct
> > eval_ctx *ctx, struct stmt *stmt)
> >  static int stmt_evaluate_log_prefix(struct eval_ctx *ctx, struct
> > stmt *stmt)
> >  {
> >         char prefix[NF_LOG_PREFIXLEN] = {}, tmp[NF_LOG_PREFIXLEN] =
> > {};
> > -       int len = sizeof(prefix), offset = 0, ret;
> > +       size_t len = sizeof(prefix);
> > +       size_t offset = 0;
> >         struct expr *expr;
> > -       size_t size = 0;
> >  
> >         if (stmt->log.prefix->etype != EXPR_LIST)
> >                 return 0;
> >  
> >         list_for_each_entry(expr, &stmt->log.prefix->expressions,
> > list) {
> > +               int ret;
> > +
> >                 switch (expr->etype) {
> >                 case EXPR_VALUE:
> >                         expr_to_string(expr, tmp);
> > @@ -4150,12 +4152,9 @@ static int stmt_evaluate_log_prefix(struct
> > eval_ctx *ctx, struct stmt *stmt)
> >                         BUG("unknown expression type %s\n",
> > expr_name(expr));
> >                         break;
> >                 }
> > -               SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
> > +               SNPRINTF_BUFFER_SIZE(ret, &len, &offset);
> >         }
> >  
> > -       if (len == NF_LOG_PREFIXLEN)
> > -               return stmt_error(ctx, stmt, "log prefix is too
> > long");
> 
> No error anymore?
> 
> Not directly related, but are you sure tests we have are sufficient
> to
> cover for all these updates 

No. I am not aware of test coverage.

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?


Thomas