Bug #74518 for AnyEvent-MPRPC: AnyEvent::MessagePack read type corrupts responses

Sun Jan 29 16:33:42 2012 asguthrie [...] gmail.com - Ticket created

Subject:	AnyEvent::MessagePack read type corrupts responses
Date:	Sun, 29 Jan 2012 21:33:32 +0000
To:	bug-AnyEvent-MPRPC [...] rt.cpan.org
From:	Adam Guthrie <asguthrie [...] gmail.com>

I'm using AnyEvent::MPRPC::Client and the deserialization of responses occasionally corrupts as described below. I get errors message like: Can't use string ("462") as an ARRAY ref while "strict refs" in use at [...]AnyEvent/MPRPC/Client.pm line 151. or Use of uninitialized value in delete at [...]AnyEvent/MPRPC/Client.pm line 151. Invalid response from server at [...]AnyEvent/MPRPC/Client.pm line 164. I believe this happens when the response is split between two on_read callbacks from the AnyEvent::Handle. In this case, the first half is forgotten and deserialization of the second half is attempted causing the messages above. Constructing a simple test case is a little difficult without a server but I attach a patch that seems to fix the issue for me. I've chosen to use the Handle's rbuf rather than Data::MessagePack::Unpacker's internal buffer as the latter is harder to use and imcomplete in some cases. I'm running perl 5.14.2 on Mac OS X 10.6.8 Adam

Message body is not shown because sender requested not to inline it.

Sun Jan 29 21:58:37 2012 tokuhirom [...] gmail.com - Correspondence added

Subject:	Re: [rt.cpan.org #74518] AnyEvent::MessagePack read type corrupts responses
Date:	Mon, 30 Jan 2012 11:58:28 +0900
To:	bug-AnyEvent-MPRPC [...] rt.cpan.org
From:	Tokuhiro Matsuno <tokuhirom [...] gmail.com>

Thanks. Can you write a test case? On Mon, Jan 30, 2012 at 6:33 AM, Adam Guthrie via RT <bug-AnyEvent-MPRPC@rt.cpan.org> wrote: Show quoted text

> Sun Jan 29 16:33:42 2012: Request 74518 was acted upon. > Transaction: Ticket created by asguthrie@gmail.com > Queue: AnyEvent-MPRPC > Subject: AnyEvent::MessagePack read type corrupts responses > Broken in: (no value) > Severity: (no value) > Owner: Nobody > Requestors: asguthrie@gmail.com > Status: new > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=74518 > > > > I'm using AnyEvent::MPRPC::Client and the deserialization of responses > occasionally corrupts as described below. I get errors message like: > > Can't use string ("462") as an ARRAY ref while "strict refs" in use at > [...]AnyEvent/MPRPC/Client.pm line 151. > > or > > Use of uninitialized value in delete at [...]AnyEvent/MPRPC/Client.pm line > 151. > Invalid response from server at [...]AnyEvent/MPRPC/Client.pm line 164. > > I believe this happens when the response is split between two on_read > callbacks from the AnyEvent::Handle. In this case, the first half is > forgotten and deserialization of the second half is attempted causing the > messages above. > > Constructing a simple test case is a little difficult without a server but > I attach a patch that seems to fix the issue for me. I've chosen to use > the Handle's rbuf rather than Data::MessagePack::Unpacker's internal buffer > as the latter is harder to use and imcomplete in some cases. > > I'm running perl 5.14.2 on Mac OS X 10.6.8 > > Adam >

Sun Jan 29 21:58:38 2012 The RT System itself - Status changed from 'new' to 'open'

Mon Jan 30 03:29:33 2012 asguthrie [...] gmail.com - Correspondence added

Subject:	Re: [rt.cpan.org #74518] AnyEvent::MessagePack read type corrupts responses
Date:	Mon, 30 Jan 2012 08:29:24 +0000
To:	bug-AnyEvent-MPRPC [...] rt.cpan.org
From:	Adam Guthrie <asguthrie [...] gmail.com>

Attached. Varying the read_size used causes different deserializations to be printed. Adam On 30 January 2012 02:58, tokuhirom@gmail.com via RT < bug-AnyEvent-MPRPC@rt.cpan.org> wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=74518 > > > Thanks. > Can you write a test case? > > On Mon, Jan 30, 2012 at 6:33 AM, Adam Guthrie via RT > <bug-AnyEvent-MPRPC@rt.cpan.org> wrote:

> > Sun Jan 29 16:33:42 2012: Request 74518 was acted upon. > > Transaction: Ticket created by asguthrie@gmail.com > > Queue: AnyEvent-MPRPC > > Subject: AnyEvent::MessagePack read type corrupts responses > > Broken in: (no value) > > Severity: (no value) > > Owner: Nobody > > Requestors: asguthrie@gmail.com > > Status: new > > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=74518 > > > > > > > I'm using AnyEvent::MPRPC::Client and the deserialization of responses > > occasionally corrupts as described below. I get errors message like: > > > > Can't use string ("462") as an ARRAY ref while "strict refs" in use at > > [...]AnyEvent/MPRPC/Client.pm line 151. > > > > or > > > > Use of uninitialized value in delete at [...]AnyEvent/MPRPC/Client.pm

> line

> > 151. > > Invalid response from server at [...]AnyEvent/MPRPC/Client.pm line 164. > > > > I believe this happens when the response is split between two on_read > > callbacks from the AnyEvent::Handle. In this case, the first half is > > forgotten and deserialization of the second half is attempted causing the > > messages above. > > > > Constructing a simple test case is a little difficult without a server

> but

> > I attach a patch that seems to fix the issue for me. I've chosen to use > > the Handle's rbuf rather than Data::MessagePack::Unpacker's internal

> buffer

> > as the latter is harder to use and imcomplete in some cases. > > > > I'm running perl 5.14.2 on Mac OS X 10.6.8 > > > > Adam > >

> >

Message body is not shown because sender requested not to inline it.

Fri Jul 20 10:43:38 2012 dolmen [...] cpan.org - Correspondence added

RT-Send-CC:

tokuhirom [...] gmail.com

I can reproduce Adam's issue. I checked that the issue is not in Data::MessagePack::Stream by writing a testcase (see https://github.com/typester/Data-MessagePack-Stream/pull/1): this is not the case. -- Olivier Mengué - http://perlresume.org/DOLMEN

Fri Jul 20 10:44:24 2012 dolmen [...] cpan.org - Requestor DOLMEN added

Thu Jul 26 12:02:21 2012 MAXS [...] cpan.org - Correspondence added

RT-Send-CC:

tokuhirom [...] gmail.com

As Olivier said, the Adam's bug can still be observed with the use of Data::MessagePack::Stream (AnyEvent-MPRPC-0.15). When we have more than one structure MessagePack-ed and we do a push_read(), a Data::MessagePack::Stream object is instanciated and is fed with *ALL* the rbuf data. When the following push_read() is called, a *NEW* Data::MessagePack::Stream object is instanciated, but can not correctly decode its stream, because the beginning of the data has been kept by the previous Data::MessagePack::Stream object. A solution that impacts only AnyEvent::MessagePack consist in keeping the Data::MessagePack::Stream instance into an AnyEvent::Handle attribute, but it is not in keeping with AnyEvent, because if we change the read_type for each push_read() we will encounter the same problem as above: the Data::MessagePack::Stream will keep data that it does not own. The attached patch implements this solution (I also modified the check for the empty rbuf to avoid leaving the sub when "0" is read, for example). The correct solution consists in modifying Data::MessagePack::Stream to make it able to handle a foreign string, taking only the data it needs to decode the current structure, or to allow it to give back the data it didn't need... Best regards, Max.

Subject:

AnyEvent-MessagePack-stream.patch

--- lib/AnyEvent/MessagePack.pm.orig 2012-07-26 17:50:46.000000000 +0200 +++ lib/AnyEvent/MessagePack.pm 2012-07-26 17:59:44.000000000 +0200 @@ -17,13 +17,13 @@ }); register_read_type(msgpack => sub { my ($self, $cb) = @_; - my $unpacker = Data::MessagePack::Stream->new(); + my $unpacker = $self->{_msgpack} ||= Data::MessagePack::Stream::->new; sub { - my $buffer = delete $_[0]{rbuf} or return; + my $buffer = delete $_[0]{rbuf}; + return unless defined $buffer and $buffer ne ''; my $complete = 0; - my $nread = 0; $unpacker->feed($buffer); while ($unpacker->next) { $cb->( $_[0], $unpacker->data );

Fri Jul 27 10:03:40 2012 dolmen [...] cpan.org - Correspondence added

RT-Send-CC:

tokuhirom [...] gmail.com

Pull request submitted on Github including Max's patch and a testcase (that fails with AnyEvent-MPRPC-0.15, but succeeds with the patch). https://github.com/tokuhirom/p5-anyevent-mprpc/pull/2 -- Olivier Mengué - http://perlresume.org/DOLMEN

Sun Jul 29 17:22:02 2012 tokuhirom+cpan [...] gmail.com - Correspondence added

Thanks, It's merged.

Sun Jul 29 17:22:03 2012 tokuhirom+cpan [...] gmail.com - Status changed from 'open' to 'resolved'