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

FixMessageDecoder.startsWith() wrongly detects pattern

    Details

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

      Description

      I have found a nasty bug in FixMessageDecoder.parseMessage() that appears when using BeginString=FIXT.1.1

      If mina provides the decoder with a ByteBuffer with an incomplete header like below the indexOf() method wrongly detects it in spite the fact the buffer does not contain a complete header.

      Below you have a test case to prove it. Unfortunately I had to copy some private methods from FixMessageDecoder in order to call them.

      import java.io.UnsupportedEncodingException;

      import junit.framework.Assert;

      import org.apache.mina.common.ByteBuffer;
      import org.junit.Test;
      import org.quickfixj.CharsetSupport;

      public class TestFixMessageDecoder {

      private static final String FIELD_DELIMITER = "\001";
      private static final byte[] HEADER_PATTERN = getBytes("8=FIXt.?.?" + FIELD_DELIMITER + "9=");

      @Test
      public void testCompleteHeader() {
      //8=FIXT.1.1_9=
      byte[] completeHeader =

      {0x38, 0x3D, 0x46, 0x49, 0x58, 0x54, 0x2E, 0x31, 0x2E, 0x31, 0x01, 0x39, 0x3D}

      ;
      ByteBuffer in = ByteBuffer.wrap(completeHeader);
      BufPos bufPos = indexOf(in, 0, HEADER_PATTERN);
      Assert.assertTrue("We should have a complete header", bufPos._offset != -1);
      }

      @Test
      public void testIncompleteHeader() {
      //8=FIXT.1.1_9
      byte[] incompleteHeader =

      {0x38, 0x3D, 0x46, 0x49, 0x58, 0x54, 0x2E, 0x31, 0x2E, 0x31, 0x01, 0x39}

      ;
      ByteBuffer in = ByteBuffer.wrap(incompleteHeader);
      BufPos bufPos = indexOf(in, 0, HEADER_PATTERN);
      Assert.assertTrue("There should be no header detected", bufPos._offset == -1);
      }

      private static byte[] getBytes(String s) {
      try

      { return s.getBytes(CharsetSupport.getDefaultCharset()); }

      catch (UnsupportedEncodingException e)

      { throw new RuntimeException(e); }

      }

      private static int startsWith(ByteBuffer buffer, int bufferOffset, byte[] data) {
      if (bufferOffset + minMaskLength(data) > buffer.limit())

      { return -1; }

      final int initOffset = bufferOffset;
      int dataOffset = 0;
      for (int bufferLimit = buffer.limit(); dataOffset < data.length
      && bufferOffset < bufferLimit; dataOffset+, bufferOffset+) {
      if (buffer.get(bufferOffset) != data[dataOffset] && data[dataOffset] != '?') {
      // Now check for optional characters, at this point we know we didn't
      // match, so we can just check to see if we failed a match on an optional character,
      // and if so then just rewind the buffer one byte and keep going.
      if (Character.toUpperCase(data[dataOffset]) == buffer.get(bufferOffset))
      continue;
      // Didn't match the optional character, so act like it was not included and keep going
      if (Character.isLetter(data[dataOffset]) && Character.isLowerCase(data[dataOffset]))

      { --bufferOffset; continue; }

      return -1;
      }
      }
      return bufferOffset - initOffset;
      }

      private static BufPos indexOf(ByteBuffer buffer, int position, byte[] data) {
      for (int offset = position, limit = buffer.limit() - minMaskLength(data) + 1; offset < limit; offset++) {
      int length;
      if (buffer.get(offset) == data[0] && (length = startsWith(buffer, offset, data)) > 0)

      { return new BufPos(offset, length); }

      }
      return new BufPos(-1, 0);
      }

      private static int minMaskLength(byte[] data) {
      int len = 0;
      for (int i = 0; i < data.length; ++i)

      { if (Character.isLetter(data[i]) && Character.isLowerCase(data[i])) continue; ++len; }

      return len;
      }

      static class BufPos {
      int _offset;
      int _length;

      /**

      • @param offset
      • @param length
        */
        public BufPos(int offset, int length) { _offset = offset; _length = length; }

      public String toString()

      { return _offset + "," + _length; }

      };
      }

      The code was taken from qfj 1.4.0

      Both test cases should pass but testIncompleteHeader does not since FixMessageDecoder.indexOf() wrongly detects a header pattern even if the header is not complete ( it's missing the last '=' character). This leads to state = PARSING_LENGTH and ends up miserably with a "Error in message length format" exception; (see the original FixMessageDecoder code)

      This bug can pass unobserved for long time since the probability mina will deliver this certain buffer to the decoder is somehow low.

        Attachments

          Issue Links

            Activity

            Hide
            seven Horia Muntean added a comment -

            We should check at the end of the method if the data[] pattern was parsed completely before returning.
            If it was not, than it means that the buffer was instead parsed. So the result should be -1.
            This reasoning works only if the unparsed pattern contains something else that optional characters.

            private static int startsWith(ByteBuffer buffer, int bufferOffset, byte[] data) {
            if (bufferOffset + minMaskLength(data) > buffer.limit())

            { return -1; }
            final int initOffset = bufferOffset;
            int dataOffset = 0;
            for (int bufferLimit = buffer.limit(); dataOffset < data.length
            && bufferOffset < bufferLimit; dataOffset+, bufferOffset+) {
            if (buffer.get(bufferOffset) != data[dataOffset] && data[dataOffset] != '?') {
            // Now check for optional characters, at this point we know we didn't
            // match, so we can just check to see if we failed a match on an optional character,
            // and if so then just rewind the buffer one byte and keep going.
            if (Character.toUpperCase(data[dataOffset]) == buffer.get(bufferOffset))
            continue;
            // Didn't match the optional character, so act like it was not included and keep going
            if (Character.isLetter(data[dataOffset]) && Character.isLowerCase(data[dataOffset])) { --bufferOffset; continue; }
            return -1;
            }
            }
            // Make sure every byte from the pattern was parsed
            if(dataOffset < data.length) { return -1; }

            return bufferOffset - initOffset;
            }

            With the proposed fix the above tests pass. I did not checkout the head, apply the fix and rerun the project tests.

            Show
            seven Horia Muntean added a comment - We should check at the end of the method if the data[] pattern was parsed completely before returning. If it was not, than it means that the buffer was instead parsed. So the result should be -1. This reasoning works only if the unparsed pattern contains something else that optional characters. private static int startsWith(ByteBuffer buffer, int bufferOffset, byte[] data) { if (bufferOffset + minMaskLength(data) > buffer.limit()) { return -1; } final int initOffset = bufferOffset; int dataOffset = 0; for (int bufferLimit = buffer.limit(); dataOffset < data.length && bufferOffset < bufferLimit; dataOffset+ , bufferOffset +) { if (buffer.get(bufferOffset) != data [dataOffset] && data [dataOffset] != '?') { // Now check for optional characters, at this point we know we didn't // match, so we can just check to see if we failed a match on an optional character, // and if so then just rewind the buffer one byte and keep going. if (Character.toUpperCase(data [dataOffset] ) == buffer.get(bufferOffset)) continue; // Didn't match the optional character, so act like it was not included and keep going if (Character.isLetter(data [dataOffset] ) && Character.isLowerCase(data [dataOffset] )) { --bufferOffset; continue; } return -1; } } // Make sure every byte from the pattern was parsed if(dataOffset < data.length) { return -1; } return bufferOffset - initOffset; } With the proposed fix the above tests pass. I did not checkout the head, apply the fix and rerun the project tests.
            Hide
            seven Horia Muntean added a comment -

            Something happened with copy&paste in the previous comment so I attached the full test file. The change was done to the startsWith() method, just before return.

            Show
            seven Horia Muntean added a comment - Something happened with copy&paste in the previous comment so I attached the full test file. The change was done to the startsWith() method, just before return.
            Hide
            sbate Steve Bate added a comment -

            I've added your tests, but they all pass. I don't think we did anything specific to fix this issue yet so that's a bit confusing. Is there some way you could run your unit tests against the trunk version of QFJ to see if you still see failing tests?

            Show
            sbate Steve Bate added a comment - I've added your tests, but they all pass. I don't think we did anything specific to fix this issue yet so that's a bit confusing. Is there some way you could run your unit tests against the trunk version of QFJ to see if you still see failing tests?
            Hide
            seven Horia Muntean added a comment -

            I am not sure to what tests you are referring to.
            The tests from the attached TestFixMessageDecoder.java pass because 'startsWith' method contains the fix.
            You should run the tests from the code taken from the description of this issue.

            Show
            seven Horia Muntean added a comment - I am not sure to what tests you are referring to. The tests from the attached TestFixMessageDecoder.java pass because 'startsWith' method contains the fix. You should run the tests from the code taken from the description of this issue.
            Hide
            davidgibbs David Gibbs added a comment -

            We see this issue as well. Is the FIX committed yet ?

            Show
            davidgibbs David Gibbs added a comment - We see this issue as well. Is the FIX committed yet ?
            Hide
            stelios.papadopoulos Stelios Papadopoulos added a comment -

            see QFJ-544 for a test that highlights this issue (without exposing the private function).

            also the tests in QuickFixJ 1.5.0 FIXMessageDecoderTest do not test the application code, but rather the private method included in the test class.

            Show
            stelios.papadopoulos Stelios Papadopoulos added a comment - see QFJ-544 for a test that highlights this issue (without exposing the private function). also the tests in QuickFixJ 1.5.0 FIXMessageDecoderTest do not test the application code, but rather the private method included in the test class.

              People

              • Assignee:
                Unassigned
                Reporter:
                seven Horia Muntean
              • Votes:
                2 Vote for this issue
                Watchers:
                2 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: