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

check the buffer length read in constant meta, if it's not equal to t… #373

Closed
wants to merge 3 commits into from

Conversation

vishal1132
Copy link

@vishal1132 vishal1132 commented May 18, 2023

There was an issue with using the LoadKnowledgeBaseFromReader function on the *ast.KnowledgeLibrary when passing the *tar.Reader which had the *gzip.Reader underlying in the r of *tar.Reader which should ideally not be passed because of how the *gzip.Reader's decompressor's read() method is implemented and how the library is not reading until EOF but actually reading a specific set of bytes, and for any such usecase the *gzip.Reader is ideal, where there is a need to read a chunk of bytes and not until EOF.
But this debugging(I believe) could be made faster if we had the buffer read check on Constant Meta's ReadMetaFrom implementation. Basically because the decompressor had some state which had the toRead method which will be updated when the step() function which takes the pointer to the decompressor and changes it's state, but since it read less number of bytes than it had to. The new 8 bytes were read from different set of bytes, which would attempt to read from a different length of bytes array. Which was the issue but could have been easier to debug if there was a check in the package (according to me.).
But in any case this doesn't compromise the correctness of the flow, and seems like a useful check. WDYT?

@@ -1535,11 +1536,16 @@ func (meta *ConstantMeta) ReadMetaFrom(reader io.Reader) error {
return err
}
byteArr := make([]byte, length)
_, err = reader.Read(byteArr)

readCount, err = reader.Read(byteArr)
Copy link
Member

Choose a reason for hiding this comment

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

I am very sorry, I committed the code wrongly. I should be
readCount, err := reader.Reader(byteArr)

Forgot to puth the collon there. Coud you change it accordingly and push again.

newm4n pushed a commit to newm4n/grule-rule-engine that referenced this pull request Aug 29, 2023
newm4n added a commit that referenced this pull request Aug 29, 2023
Co-authored-by: Ferdinand Neman <ferdinand@hyperjump.tech>
@newm4n
Copy link
Member

newm4n commented Aug 29, 2023

I have accepted this pull request but my attempt to merge it fails.
Its my mistakes, thus I copy you change directly and incorporate it into master.
Thanks a lot for your contribution.

@newm4n newm4n closed this Aug 29, 2023
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