[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: | TestFixMessageDecoder.java | ||||||||
Issue Links: |
|
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; public class TestFixMessageDecoder { private static final String FIELD_DELIMITER = "\001"; @Test ; @Test ; private static byte[] getBytes(String s) { catch (UnsupportedEncodingException e) { throw new RuntimeException(e); }} private static int startsWith(ByteBuffer buffer, int bufferOffset, byte[] data) { final int initOffset = bufferOffset; return -1; private static BufPos indexOf(ByteBuffer buffer, int position, byte[] data) { } private static int minMaskLength(byte[] data) { return len; static class BufPos { /**
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. private static int startsWith(ByteBuffer buffer, int bufferOffset, byte[] data) { 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. |
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 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. |