Mailing-List: contact commits-help@cassandra.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@cassandra.apache.org
Date: Wed, 22 Jul 2015 11:01:05 +0000 (UTC)
From: "Benedict (JIRA)" <jira@apache.org>
To: commits@cassandra.apache.org
Message-ID: <JIRA.12846857.1437561528000.255241.1437562865180@Atlassian.JIRA>
In-Reply-To: <JIRA.12846857.1437561528000@Atlassian.JIRA>
References: <JIRA.12846857.1437561528000@Atlassian.JIRA>
 <JIRA.12846857.1437561528069@arcas>
Subject: [jira] [Comment Edited] (CASSANDRA-9863) NIODataInputStream can
 loop forever
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


    [ https://issues.apache.org/jira/browse/CASSANDRA-9863?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14636716#comment-14636716 ] 

Benedict edited comment on CASSANDRA-9863 at 7/22/15 11:01 AM:
---------------------------------------------------------------

That looks like actually a very simple bug: we should return immediately if {{read == -1}} and {{remaining >= attempt}}. This is a different problem to the one in CASSANDRA-9708.

The testing should certainly have caught this (as should I in review), as it's a pretty basic requirement of the contract.

Comments can certainly always be improved, although it isn't always clear upfront where the confusion will be (more of a problem when both reviewer and author are more closely involved in development, as in this case). In the case of {{readMinimum}} the hope was that the parameter names would be suffiicent. Both parameters are minima, but one is _enforced_ and the other is _desired_. i.e. we {{attempt}} to read a minimum, and fail if we get less than {{required}}. Perhaps somebody else should take review of updates to the comments in this instance, to ensure we don't repeat the mistake of too much knowledge weakening the explanation.


was (Author: benedict):
That looks like actually a very simple bug: we should return immediately if {{read == -1}} and {{remaining >= require}}. This is a different problem to the one in CASSANDRA-9708.

The testing should certainly have caught this (as should I in review), as it's a pretty basic requirement of the contract.

Comments can certainly always be improved, although it isn't always clear upfront where the confusion will be (more of a problem when both reviewer and author are more closely involved in development, as in this case). In the case of {{readMinimum}} the hope was that the parameter names would be suffiicent. Both parameters are minima, but one is _enforced_ and the other is _desired_. i.e. we {{attempt}} to read a minimum, and fail if we get less than {{required}}. Perhaps somebody else should take review of updates to the comments in this instance, to ensure we don't repeat the mistake of too much knowledge weakening the explanation.

> NIODataInputStream can loop forever
> -----------------------------------
>
>                 Key: CASSANDRA-9863
>                 URL: https://issues.apache.org/jira/browse/CASSANDRA-9863
>             Project: Cassandra
>          Issue Type: Bug
>            Reporter: Sylvain Lebresne
>            Assignee: Ariel Weisberg
>            Priority: Blocker
>             Fix For: 3.0 beta 1
>
>
> As the title says, there is cases where method calls to NIODataInputStream, at least {{readVInt}} calls can loop forever. This is possibly only a problem for vints where the code tries to read 8 bytes minimum but there is less than that available, and in that sense is related to [~benedict]'s observation in CASSANDRA-9708, but it is more serious than said observation because:
> # this can happen even if the buffer passed to NIODataInputStream ctor has more than 8 bytes available, and hence I'm relatively confident [~benedict]'s fix in CASSANDRA-9708 is not enough.
> # this doesn't necessarily fail cleanly by raising assertions, this can loop forever (which is much harder to debug).
> Due of that, and because that is at least one of the cause of CASSANDRA-9764, I think the problem warrants a specific ticket (from CASSANDRA-9708 that is).
> Now, the exact reason of this is looping is if {{readVInt}} is called but the buffer has less than 8 byte remaining (again, the buffer had more initially). In that case, {{readMinimum(8, 1)}} is called and it calls {{readNext()}} in a loop. Within {{readNext()}}, the buffer (which has {{buf.position() == 0 && buf.hasRemaining()}}) is actually unchanged (through a very weird dance of setting the position to the limit, then the limit to the capacity, and then flipping the buffer which resets everything to what it was), and because {{rbc}} is the {{emptyReadableByteChannel}}, {{rbc.read(buf)}} does nothing and always return {{-1}}. Back in {{readMinimum}}, {{read == -1}} but {{remaining >= require}} (and {{remaining}} never changes), and hence the forever looping.
> Now, not sure what the best fix is because I'm not fully familiar with that code, but that does leads me to a 2nd point: {{NIODataInputSttream}} can IMHO use a bit of additional/better comments. I won't pretend having tried very hard to understand the whole class, so there is probably some lack of effort, but at least a few things felt like they should clarified:
> * Provided I understand {{readNext()}} correctly, it only make sense when we do have a {{ReadableByteChannel}} (and the fact that it's not the case sounds like the bug). If that's the case, this should be explicitly documented and probably asserted. As as an aside, I wonder if using {{rbc == null}} when we don't have wouldn't be better: if we don't have one, we shouldn't try to use it, and having a {{null}} would make things fail loudly if we do.
> * I'm not exactly sure what {{readMinimum}} arguments do. I'd have expected at least one to be called "minimum", and an explanation of the meaning of the other one.
> * {{prepareReadPaddedPrimitive}} says that it "Add padding if requested" but there is seemingly no argument that trigger the "if requested part". Also unclear what that padding is about in the first place.
> As a final point, it looks like the case where {{NIODataInputStream}} is constructed with a {{ByteBuffer}} (rather than a {{ReadableByteChannel}}) seems to be completely untested by the unit tests.


--
This message was sent by Atlassian JIRA
(v6.3.4#6332)