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.
Probably it time to turn this into a real function?
> #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?
> -
> expr = constant_expr_alloc(&stmt->log.prefix->location, &string_type,
> BYTEORDER_HOST_ENDIAN,
> strlen(prefix) * BITS_PER_BYTE, prefix);
> diff --git a/src/meta.c b/src/meta.c
> index 4f383269d032..6dada9719e5b 100644
> --- a/src/meta.c
> +++ b/src/meta.c
> @@ -999,11 +999,11 @@ struct error_record *meta_key_parse(const struct location *loc,
> const char *str,
> unsigned int *value)
> {
> - int ret, len, offset = 0;
> const char *sep = "";
> + size_t offset = 0;
> unsigned int i;
> char buf[1024];
> - size_t size;
> + size_t len;
>
> for (i = 0; i < array_size(meta_templates); i++) {
> if (!meta_templates[i].token || strcmp(meta_templates[i].token, str))
> @@ -1026,9 +1026,10 @@ struct error_record *meta_key_parse(const struct location *loc,
> }
>
> len = (int)sizeof(buf);
> - size = sizeof(buf);
>
> for (i = 0; i < array_size(meta_templates); i++) {
> + int ret;
> +
> if (!meta_templates[i].token)
> continue;
>
> @@ -1036,8 +1037,7 @@ struct error_record *meta_key_parse(const struct location *loc,
> sep = ", ";
>
> ret = snprintf(buf+offset, len, "%s%s", sep, meta_templates[i].token);
> - SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
> - assert(offset < (int)sizeof(buf));
> + SNPRINTF_BUFFER_SIZE(ret, &len, &offset);
> }
>
> return error(loc, "syntax error, unexpected %s, known keys are %s", str, buf);
> --
> 2.41.0
>
