-
Notifications
You must be signed in to change notification settings - Fork 91
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
base: master
Are you sure you want to change the base?
Conversation
Roman-Koshelev
commented
Apr 11, 2021
- Strengthen the checks (for example, check that the separating symbol is '='
- We will detect invalid messages as soon as possible
TODO
|
Reviewing... |
include/hffix.hpp
Outdated
++b; | ||
} | ||
// look for the first '\x01' | ||
b = (const char*)memchr(b, '\x01', buffer_end_ - b); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ...
include/hffix.hpp
Outdated
// 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(); |
There was a problem hiding this comment.
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; }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree
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.
I think it's already true that if a tag has no corresponding value, then the message will be considered invalid?
What kind of hash sum check? We already have the FIX checksum. |
|
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
Update |
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 |