lkml.org 
[lkml]   [2006]   [Jan]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [V9fs-developer] [PATCH 3/3] v9fs: zero copy implementation
On Thu, Jan 05, 2006 at 10:13:52AM -0500, Russ Cox said:
> > +char *v9fs_str_copy(char *buf, int buflen, struct v9fs_str *str)
> > +{
> > + int n;
> > +
> > + if (buflen < str->len)
> > + n = buflen;
> > + else
> > + n = str->len;
> > +
> > + memmove(buf, str->str, n - 1);
> > +
> > + return buf;
> > +}
>
>
> The above is wrong. You'll chop the end of the string off
> when str->len <= buflen.

You are right.

>
> n = str->len;
> if (n > buflen-1)
> n = buflen-1;
> memmove(buf, str->str, n);
> buf[n] = 0;
>
> > +int v9fs_str_compare(char *buf, struct v9fs_str *str)
> > +{
> > + int n, ret;
> > +
> > + ret = strncmp(buf, str->str, str->len);
> > +
> > + if (!ret) {
> > + n = strlen(buf);
> > + if (n < str->len)
> > + ret = -1;
> > + else if (n > str->len)
> > + ret = 1;
> > + }
> > +
> > + return ret;
> > +}
>
> You go through all this work to avoid copying the strings,
> which has questionable benefit, and then this routine
> walks the length of the string twice, unnecessarily.
> Also if strlen(buf) < str->len, then strncmp can't return 0.

I avoid copying the strings not because of the copy itself, but to not
bother freeing them when v9fs_fcall structure is freed. You are right, the
logic is wrong. I was trying (obviously unsuccessfully) to match strcmp
semantics, although I only use the function to check for equality of the
strings.

>
> ret = strncmp(buf, str->str, str->len);
> if (!ret && buf[str->len])
> ret = 1;
> return ret;
>
> > static inline int buf_check_size(struct cbuf *buf, int len)
> > {
> [snip deleted lines]
> > + if (buf->p + len > buf->ep && buf->p < buf->ep) {
> > + eprintk(KERN_ERR, "buffer overflow: want %d has %d\n",
> > + len, (int)(buf->ep - buf->p));
> > + dump_stack();
> > + buf->p = buf->ep + 1;
> > + return 0;
> > }
> >
> > return 1;
>
> I think it's weird that you return 1 when you've already overflowed.
> It's fine that you don't print more than once, but what's the harm
> in returning 0 always when buf->p + len > buf->ep?

It is not just weird, it is wrong and can lead to writing outside of the
buffer.

Thanks for reviewing the code.

Lucho
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2006-01-06 03:04    [W:0.055 / U:7.584 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site