On 4/10/18 12:50 PM, Rasmus Villemoes wrote:
> 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.
Right, ok.
> 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.
There's a lot that can be fixed within reiserfs. I've started down
enough of the rabbit holes to know to avoid them.
>> 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.
I'm not concerned about the spinlock cost. This is the fault path, not
a fast path. The other two things are valid and fixed in patch 4, but
I'll fix them here too.
-Jeff
--
Jeff Mahoney
SUSE Labs
--
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
