On Mon 09-04-18 14:16:02, jeffm@xxxxxxxx wrote:
> From: Jeff Mahoney <jeffm@xxxxxxxx>
>
> reiserfs_snprintf operates by handling the reiserfs-specific specifiers
> itself and passing everything else to the generic vsnprintf routine.
> That makes some assumptions about what the state of a va_list passed
> by value to another function will be after the function returns. The
> spec states that the contents of the va_list are undefined in that
> case. It's worked so far but really only by the grace of gcc's internal
> va_list implementation details.
>
> This patch reworks reiserfs_snprintf to process one % directive at
> a time. Since directives can consume more than one argument when field
> width and precision * operators are used, that means we need to interpret
> the format string more than we used to do. (The kernel vsnprintf
> specifically doesn't handle %n). Otherwise, we can use an unsigned long
> to hold the argument and let the generic snprintf do the type handling.
> The only exception is long long arguments on platforms where long long
> is larger than long, which need special handling.
So I wonder why is this correct for architectures where sizeof(int) !=
sizeof(long). If you do va_arg(va, long) for an argument which is actually
int, you'll consume more bytes from the stack than you should. Or am I
missing some va_arg() subtlety?
> The result is a va_list that is consistent within reiserfs_snprintf
> and isn't passed by value anymore.
>
> This isn't an ideal solution, but I didn't feel that teaching pointer()
> about a %pV variant that didn't use va_copy would get much traction for
> a number of reasons.
Frankly, I don't like the format string parsing duplication in your patch
and I'm afraid it will be a maintenance issue. Can't we have a variant
of vsnprintf() that would take a pointer to va_list (like you use in
your patch for some functions) so that its contents is well defined after
the function returns?
Honza
>
> Reported-by: Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Jeff Mahoney <jeffm@xxxxxxxx>
> ---
> fs/reiserfs/prints.c | 158 +++++++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 128 insertions(+), 30 deletions(-)
>
> diff --git a/fs/reiserfs/prints.c b/fs/reiserfs/prints.c
> index aead27d11e45..03ebdea9ddc7 100644
> --- a/fs/reiserfs/prints.c
> +++ b/fs/reiserfs/prints.c
> @@ -6,6 +6,7 @@
> #include <linux/fs.h>
> #include "reiserfs.h"
> #include <linux/string.h>
> +#include <linux/ctype.h>
> #include <linux/buffer_head.h>
>
> #include <stdarg.h>
> @@ -195,19 +196,94 @@ static size_t snprintf_disk_child(char *buf, size_t size, struct disk_child *dc)
> dc_block_number(dc), dc_size(dc));
> }
>
> -static char *is_there_reiserfs_struct(char *fmt, int *what)
> -{
> - char *k = fmt;
> -
> - while ((k = strchr(k, '%')) != NULL) {
> - if (k[1] == 'k' || k[1] == 'K' || k[1] == 'h' || k[1] == 't' ||
> - k[1] == 'z' || k[1] == 'b' || k[1] == 'y' || k[1] == 'a') {
> - *what = k[1];
> +/*
> + * This should all just be %pV but pointer() does a va_copy and uses that
> + * instead of directly using args. That's the right thing to do pretty
> + * much every time, but it still forces us to identify how many arguments
> + * we need to pass for a single format spec. We need to be careful of u64
> + * on 32-bit since it'll need special handling. We can just use an
> + * unsigned long for everything else and vsnprintf will handling the
> + * typing itself.
> + */
> +static int handle_generic_snprintf(char *p, size_t left,
> + const char *fmt, va_list *args)
> +{
> + int width, precision, nargs = 1;
> + const char *spec = fmt;
> + unsigned long val;
> + int len = 0;
> +
> + /* Skip flags */
> + while (1) {
> + bool found = true;
> + ++spec;
> + switch (*spec) {
> + case '-':
> + case '+':
> + case ' ':
> + case '#':
> + case '0':
> break;
> + default:
> + found = false;
> }
> - k++;
> + if (!found)
> + break;
> + }
> +
> + /* Field width */
> + if (isdigit(*spec)) {
> + while (isdigit(*++spec));
> + } else if (*spec == '*') {
> + nargs++;
> + spec++;
> + }
> +
> + /* Precision */
> + if (*spec == '.') {
> + spec++;
> + if (isdigit(*spec)) {
> + while (isdigit(*++spec));
> + } else if (*spec == '*') {
> + nargs++;
> + spec++;
> + }
> + }
> +
> + if (*spec == 'L' || !strncmp(spec, "ll", 2)) {
> + unsigned long long ullval;
> + if (nargs == 3) {
> + width = va_arg(*args, int);
> + precision = va_arg(*args, int);
> + ullval = va_arg(*args, unsigned long long);
> + len = snprintf(p, left, fmt, width, precision,
> + ullval);
> + } else if (nargs == 2) {
> + width = va_arg(*args, int);
> + ullval = va_arg(*args, unsigned long long);
> + len = snprintf(p, left, fmt, width, ullval);
> + } else {
> + ullval = va_arg(*args, unsigned long long);
> + len = snprintf(p, left, fmt, ullval);
> + }
> + return len;
> + }
> +
> + if (nargs == 3) {
> + width = va_arg(*args, int);
> + precision = va_arg(*args, int);
> + val = va_arg(*args, unsigned long);
> + len = snprintf(p, left, fmt, width, precision, val);
> + } else if (nargs == 2) {
> + width = va_arg(*args, int);
> + val = va_arg(*args, unsigned long);
> + len = snprintf(p, left, fmt, width, val);
> + } else {
> + val = va_arg(*args, unsigned long);
> + len = snprintf(p, left, fmt, val);
> }
> - return k;
> +
> + return len;
> }
>
> /*
> @@ -237,23 +313,34 @@ static char fmt_buf[1024];
> static int reiserfs_snprintf(char *p, size_t left, const char *fmt,
> va_list *args)
> {
> + size_t fmtlen = strlen(fmt);
> + const char *fmtend = fmt + fmtlen;
> + const char *end;
> char *start = p;
> - char *fmt1 = fmt_buf;
> - char *k;
> - int what;
> - size_t len;
>
> - spin_lock(&fmt_lock);
> - strncpy(fmt1, fmt, sizeof(fmt_buf));
> + while (left && fmt != NULL && *fmt) {
> + bool reiserfs_spec = true;
> + int fmt_size;
> + size_t len;
>
> - while ((k = is_there_reiserfs_struct(fmt1, &what)) != NULL) {
> - *k = 0;
> + end = strchr(fmt + 1, '%');
>
> - len = vsnprintf(p, left, fmt1, *args);
> - p += len;
> - left -= len;
> + /* Skip over %% */
> + while (end && !strncmp(end, "%%", 2))
> + end = strchr(end + 2, '%');
> +
> + if (!end)
> + end = fmtend;
>
> - switch (what) {
> + fmt_size = end - fmt;
> +
> + /* Any text before the first % - could be entire string */
> + if (fmt[0] != '%') {
> + len = snprintf(p, left, "%.*s", fmt_size, fmt);
> + goto next;
> + }
> +
> + switch (fmt[1]) {
> case 'k':
> len = snprintf_le_key(p, left,
> va_arg(*args, struct reiserfs_key *));
> @@ -286,19 +373,30 @@ static int reiserfs_snprintf(char *p, size_t left, const char *fmt,
> len = snprintf_de_head(p, left,
> va_arg(*args, struct reiserfs_de_head *));
> break;
> - }
> + default:
> + spin_lock(&fmt_lock);
> + strncpy(fmt_buf, fmt, fmt_size);
> + fmt_buf[fmt_size] = 0;
>
> + len = handle_generic_snprintf(p, left, fmt_buf, args);
> + spin_unlock(&fmt_lock);
> +
> + reiserfs_spec = false;
> + break;
> + };
> +
> + if (reiserfs_spec) {
> + p += len;
> + left -= len;
> +
> + len = snprintf(p, left, "%.*s", fmt_size - 2, fmt + 2);
> + }
> +next:
> p += len;
> left -= len;
> - fmt1 = k + 2;
> + fmt = end;
> }
>
> - len = vsnprintf(p, left, fmt1, *args);
> - p += len;
> - left -= len;
> -
> - spin_unlock(&fmt_lock);
> -
> return p - start;
> }
>
> --
> 2.12.3
>
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe reiserfs-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
