Re: [Libevent-users] [PATCH] evbuffer_search_range fails when searching the last byte

[Nir Soffer <nirsof@xxxxxxxxx>]

On Jun 14, 2011, at 12:06 AM, Nick Mathewson wrote:

> On Wed, Jun 8, 2011 at 8:38 PM, Nir Soffer <nirsof@xxxxxxxxx> wrote:
> [...]
> > > Also, there's another problem with my original patch.  By my  
> > > logic, if
> > > the buffer is empty and has no chains at all, then
> > > evbuffer_ptr_set(buf, ptr, 0, EVBUFFER_PTR_SET) ought to work, but
> > > even with my patch it still doesn't.  More work needed, it would  
> > > seem.
> >
> > I think I have a working patch.
> >
> > The special "end of buffer" ptr is:
> >
> >        ptr.pos = buffer->total_len
> >        ptr._internal.chain = NULL
> >        ptr._internal.pos_in_chain = 0
> >
> > This is the first byte in the nonexistent chain after last chain.  
> > This works
> > with both empty buffer without any chain, and buffer with some  
> > chains.
>
> This, with your change to fix the race conditions, looks good to me.
> I've got a few tweaks to apply on top of it, and I've split out the
> parts that were already in my patch from the ones that weren't, for
> git clarit.  Now the whole mess is in my github repository, in a
> branch called "21_end_of_buffer" in
> git://github.com/nmathewson/Libevent.git .
>
> [...]
> > There is one dark corner in the internal evbuffer_getchar - if  
> > called with
> > the "end" ptr, it returns -1, which may be valid char on system  
> > where char
> > is unsigned. It is used in only one place, using found ptr, which  
> > cannot be
> > the end ptr, so it will not cause any problem with current code.
>
> Well, because char is going to be 8 bits, ((char)-1) *will* be cast to
> a valid character everywhere.  I've changed it to follow the getchar
> interface of returning either an unsigned char or returning -1.

Changing the return value to int solves the issue nicely and more  
consistent with the standard library.

> > There is some duplication in evbuffer_peek, returning nothing if  
> > peeking at
> > the end of the buffer. I did not want to enter the critical section  
> > in this
> > case.
> >
> > The internal evbuffer_substract works, but is not efficient in this  
> > case -
> > it will search from the first chain since the end ptr chain is  
> > NULL. This
> > code is used once with found ptr, so it should not be a problem.
>
> Hm.  Just to be sure, I'm having this check the same condition as the
> other code (_internal.chain==NULL).

Better.

> > I added tests for searching and peeking at the end of the buffer  
> > and setting
> > a ptr on an empty buffer with no chains.
> >
> > I did not check yet other code that may use evbuffer_ptr outside  
> > buffer.c.
> >
> > What do you think?
>
> If you like my tweaks, I think this is a branch ready to merge onto  
> 2.1.

I think they are good.

Why 2.1 and not next bug fix release for 2.0?

In https://github.com/nmathewson/Libevent/commit/9ab8ab83cd87d3733674e8b483d823eab3ff531b
You set the value of _internal.pos_in_chain when returning the "not  
found" pointer. There are other places that do not set it. The  
attached patch set all the fields when not found pointer is returned.

Attachment: 0006-Set-the-special-not-found-evbuffer_ptr-consistantly.patch
Description: Binary data

Now, I wonder if we can unify the "not found" and "end of buffer"  
pointers - what if both set .pos to -1?

Then this may work without checking the buffer length or  
evbuffer_ptr_set return value:

struct evbuffer_ptr ptr, end;
struct evbuffer *buf = evbuffer_new();
evbuffer_add_printf(buf, "%s", "foo");

/* This will fail but set end to "not found" which is now identical to  
"end of buffer" ptr */
evbuffer_ptr_set(buf, &end, 7, EVBUFFER_PTR_SET);

ptr = evbuffer_search_range(buf, "foo", 3, NULL, &end);


Regards,
Nir