On 03/14/2013 05:50 PM, Amos Jeffries wrote:
> On 15/03/2013 12:04 p.m., Alex Rousskov wrote:
>> The code also did not terminate the remaining replies correctly after moving
>> them to the beginning of the buffer. As far as I could test, such
>> termination is accidentally(?) not necessary,
> Intentionally. It is a new parser loop. All the parser changes I've made
> have intentionally used explicit string lengths whenever possible to
> avoid dependency on correct termination (or which byte).
The "new loop" was still using good old strchr() right after the buggy
memmove() so there was a dependency on correct termination AFAICT. Every
time we change srv->roffset, we should re-terminate and this place was
missing that re-termination.
> while ((t = strchr(srv->rbuf, hlp->eom))) {
...
> *t = '\0'; // terminate at the EoM marker of the current reply
...
> srv->roffset -= (t - srv->rbuf) + skip;
> memmove(srv->rbuf, t + skip, srv->roffset); // overwrite termination
> // now rbuf is not terminated where one would expect it to be
> }
The buggy memmove aside, the loop still worked because we were usually
moving the EoM marker (from the next reply) and strchr() would find that
marker instead of scanning the already processed replies after srv->roffset.
What I could not understand is why it worked after the last iteration
when srv->roffset becomes zero and nothing should be memmoved. Perhaps
it worked because there were no EoMm markers left? Still, strchr() would
have to scan the whole original rbuf to arrive at the conclusion so the
code would be incorrect without the added termination (but accidentally
worked).
Note that v3.2 memmoved the \0 terminator so it probably needs none of
the two fixes in r12723.
> Okay. For now that is fine. It just means we will have to remove it
> again when SBuf integration happens.
Sure, along with memmove(), explicit srv->roffset updates, and many
other things.
Alex.
Received on Fri Mar 15 2013 - 16:25:26 MDT
This archive was generated by hypermail 2.2.0 : Sat Mar 16 2013 - 12:00:07 MDT