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@xxxxxxxxxx>
> > ---
> > 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
