[QFJ-505] FixMessageDecoder.startsWith() wrongly detects pattern Created: 26/Jan/10  Updated: 31/Jul/12  Resolved: 31/Jul/12

Status: Closed
Project: QuickFIX/J
Component/s: None
Affects Version/s: 1.4.0
Fix Version/s: None

Type: Bug Priority: Default
Reporter: Horia Muntean Assignee: Unassigned
Resolution: Duplicate Votes: 2
Labels: None

Attachments: Text File TestFixMessageDecoder.java    
Issue Links:
Duplicate
duplicates QFJ-544 Header with BeginString=FIXT.1.1 inco... Resolved

 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.



 Comments   
Comment by Horia Muntean [ 26/Jan/10 ]

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.

Comment by Horia Muntean [ 27/Jan/10 ]

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.

Comment by Steve Bate [ 05/Apr/10 ]

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?

Comment by Horia Muntean [ 07/Apr/10 ]

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.

Comment by David Gibbs [ 12/Jul/10 ]

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

Comment by Stelios Papadopoulos [ 20/Jul/10 ]

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.

Generated at Sat Nov 23 11:48:09 UTC 2024 using JIRA 7.5.2#75007-sha1:9f5725bb824792b3230a5d8716f0c13e296a3cae.