[QFJ-557] Session.generateReject methods may incorrectly increment nextTargetMsgSeqNum in some scenarios Created: 10/Sep/10 Updated: 02/Apr/15 Resolved: 07/Nov/13 |
|
Status: | Closed |
Project: | QuickFIX/J |
Component/s: | Engine |
Affects Version/s: | 1.4.0 |
Fix Version/s: | 1.6.0 |
Type: | Bug | Priority: | Default |
Reporter: | Rhys Yarranton | Assignee: | Christoph John |
Resolution: | Fixed | Votes: | 0 |
Labels: | None |
Attachments: | diff.txt QFTest.java |
Description |
Scenario is as follows:
We were able to write a test to reproduce this. Tracing it through the debugger led to the following snippet in generateReject(Message, int, int):
if (!msgType.equals(MsgType.LOGON) && !msgType.equals(MsgType.SEQUENCE_RESET)
&& (msgSeqNum == getExpectedTargetNum() || !isPossibleDuplicate(message))) {
state.incrNextTargetMsgSeqNum();
(There is a similar snippet in the other generateReject method.) The "bad" message 228 is not a Logon, not a SequenceReset and not a possible duplicate. Therefore the nextTargetMsgSeqNum is incremented, even though it should not be. Consequently the Session thinks message 223 has too low a sequence number. Normally this would cause a log message and disconnect, but since it is flagged as a possible duplicate and is a SequenceReset, the message is silently dropped instead. See doTargetTooLow(Message) and validatePossDup(Message). At this point we do not have a proposed fix. However, it seems clear that message 228 should not have caused an increment to nextTargetMsgSeqNum. The section "|| !isPossibleDuplicate(message)" in the snippet quoted above is a tad mysterious. Or it could be something deeper about how dictionary validation and sequence number validation should be ordered. |
Comments |
Comment by Rhys Yarranton [ 10/Sep/10 ] |
Test case attached. |
Comment by Rhys Yarranton [ 16/Sep/10 ] |
After looking at the spec and the history of the source code in question, I suspect that the logic to increment the next expected target sequence number was taken rather literally from the spec, and then a couple of special cases were removed. Attached is a patch that changes it to only increment if the current message has the expected sequence number. You can think of this as continuing the tradition of the existing logic. The spec makes an implicit assumption that the message in question is in order. You could take that as an argument in favour of reordering of the logic, but it is also possible that the authors of the spec simply didn't envisage this case. Personally I think re-ordering the logic has merit. |
Comment by Christoph John [ 07/Nov/13 ] |
Committed as rev 1116: http://sourceforge.net/p/quickfixj/code/1116/ |