Uploaded image for project: 'QuickFIX/J'
  1. QuickFIX/J
  2. QFJ-557

Session.generateReject methods may incorrectly increment nextTargetMsgSeqNum in some scenarios

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Default
    • Resolution: Fixed
    • Affects Version/s: 1.4.0
    • Fix Version/s: 1.6.0
    • Component/s: Engine
    • Labels:
      None

      Description

      Scenario is as follows:

      • expecting 223
      • received 227 Logon, which we accepted --> "MsgSeqNum too high, expecting 223 but received 227"
      • sent resend request from 223 to 0
      • received 228, which failed data dictionary validation. --> "Message 228 Rejected: Invalid MsgType"
      • sent a Reject
      • received 223 SequenceReset, w/ PossDupFlag=Y, GapFillFlag=Y and NextSeqNo=225. This gap fill was not applied.
      • received 225 ExecutionReport w/ PossDupFlag=Y. --> "Rejected MsgSeqNum too high, expecting 224 but received 225", and "Already sent ResendRequest FROM: 223 TO: 226. Not sending another."

      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.

        Attachments

        1. diff.txt
          1 kB
          Rhys Yarranton
        2. QFTest.java
          3 kB
          Rhys Yarranton

          Activity

            People

            • Assignee:
              chrjohn Christoph John
              Reporter:
              ryarran Rhys Yarranton
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: