On 2018-04-09 20:15, jeffm@xxxxxxxx wrote:
> From: Jeff Mahoney <jeffm@xxxxxxxx>
>
> reiserfs uses sprintf to print error messages and never does any bounds
> checking. This patch uses snprintf everywhere, does proper length
> tracking, and gets rid of a few copies to static buffers along the way.
You're using the wrong pattern. You should be using scnprintf rather
than snprintf, otherwise the overflow checking won't do what you expect
it to. I.e., if one of the snprintf actually overflow the buffer, it
still returns the (standard-mandated) value of the number of bytes that
would have been printed (less the terminating \0) had the buffer been
big enough. So when you then increase buf and decrease size by that
amount, buf ends up pointing past the buffer, and size becomes some huge
positive number.
We have a WARN in vsnprintf to catch that kind of abuse, so all
subsequent snprintf calls will return 0, but there's no need to trigger
that WARN at all.
Just to repeat myself from other threads on the same issue: scnprintf()
has the property that, when provided a positive size argument, it always
returns something strictly less than that, so
ret = scnprintf(buf, size, ...);
buf += ret;
size -= ret;
may eventually reduce size to 1, but not beyond that. Sometimes the
pattern is instead spelled
len += scnprintf(buf + len, size - len, ...)
(i.e. with buf and size kept fixed, just keeping tally of the total
length printed), but the same principle applies.
A few other comments inline (I haven't read the entire patch too
closely, just the snprintf pattern jumped out)
>
> -static void sprintf_direntry(char *buf, struct reiserfs_dir_entry *de)
> +static size_t snprintf_direntry(char *buf, size_t size,
> + struct reiserfs_dir_entry *de)
> {
> char name[20];
>
> memcpy(name, de->de_name, de->de_namelen > 19 ? 19 : de->de_namelen);
> name[de->de_namelen > 19 ? 19 : de->de_namelen] = 0;
> - sprintf(buf, "\"%s\"==>[%d %d]", name, de->de_dir_id, de->de_objectid);
> + return snprintf(buf, size, "\"%s\"==>[%d %d]", name,
> + de->de_dir_id, de->de_objectid);
> }
Independent of the sprintf/snprintf/scnprintf thing, just use "%.19s"
and make this whole function a oneliner. Or if de->de_name is not
nul-terminated, make that "%.*s" and pass min_t(int, 19, de->de_namelen)
for the * argument.
>
> static char *is_there_reiserfs_struct(char *fmt, int *what)
> @@ -190,56 +233,61 @@ static void prepare_error_buf(const char *fmt, va_list args)
> char *k;
> char *p = error_buf;
> int what;
> + size_t left = sizeof(error_buf);
> + size_t len;
>
> spin_lock(&error_lock);
>
> - strcpy(fmt1, fmt);
> + strncpy(fmt1, fmt, sizeof(fmt_buf));
Really? fmt_buf is 1024 bytes, so this introduces a rather slow (at
least, I don't think anybody has cared about optimizing strncpy)
memset(, 0, around-1000-bytes) inside a spinlock. Also, strncpy doesn't
nul-terminate if we do overflow, and even if it did (i.e., if you use
strlcpy instead), this could break a %something specifer in the middle.
I don't have a good suggestion, but from the "doesn't nul-terminate"
alone, the above is definitely not what you want to do.
Rasmus
--
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
