Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Let's rewrite the checks in message_reader::init() #40

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Roman-Koshelev
Copy link

  1. Strengthen the checks (for example, check that the separating symbol is '='
  2. We will detect invalid messages as soon as possible

@Roman-Koshelev
Copy link
Author

TODO

  1. Add checks that tags are numbers
  2. Add a check that each tag has a corresponding value (and remove this check from the iterator increment)
  3. Add hash sum check

@jamesdbrock
Copy link
Owner

Reviewing...

++b;
}
// look for the first '\x01'
b = (const char*)memchr(b, '\x01', buffer_end_ - b);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way this worked before is that we only looked for the \x01 in the first 11 bytes, because the longest possible version prefix string is "8=FIXT.1.1". I think we should keep that behavior, and not look for \x01 in the entire buffer.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why to take the maximum existing length. And if tomorrow there is a new version one character longer? I'll return it as it was ...

// Skip the version prefix string "8=FIX.4.2" or "8=FIXT.1.1", et cetera.
char const* b = buffer_ + 9; // look for the first '\x01'
if (b >= buffer_end_) return;
if (*b++ != '8') return invalid();
Copy link
Owner

@jamesdbrock jamesdbrock Apr 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The invalid() function performs a mutation, it has no return value. This also returns nothing, but it makes it look like we care about the return value of invalid(). I prefer { invalid(); return; }

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree

@jamesdbrock
Copy link
Owner

TODO

  1. Add checks that tags are numbers

Yeah that's probably a good idea. I used to think that this was a waste of CPU cycles but I've changed my mind.

  1. Add a check that each tag has a corresponding value (and remove this check from the iterator increment)

I think it's already true that if a tag has no corresponding value, then the message will be considered invalid?

  1. Add hash sum check

What kind of hash sum check? We already have the FIX checksum.

@Roman-Koshelev
Copy link
Author

  1. The checksum we do not verify (

Strengthen the check, and also remove the
restriction on the length of the value
We will strengthen the check, and also detect an
invalid message for as few characters as possible
We will strengthen the check, and also detect an
invalid message for as few characters as possible
@Roman-Koshelev
Copy link
Author

Update

@jamesdbrock
Copy link
Owner

  1. The checksum we do not verify (

We have the option to verify the checksum though. We leave it to the library user to decide if they want to verify the checksum. https://jamesdbrock.github.io/hffix/classhffix_1_1message__reader.html#ad3d5b61394355c230321018cdc38a57b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants