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

Fixed error when reading out the data #81

Closed
wants to merge 2 commits into from

Conversation

steffi3011
Copy link

The variable "count" is now set correctly.

The variable "count" is now set correctly.
Fixed error when reading out the data
@steffi3011
Copy link
Author

#80 Error reading the data

@ironfede ironfede self-requested a review February 8, 2021 22:48
@ironfede ironfede self-assigned this Feb 8, 2021
@ironfede ironfede added the bug label Feb 8, 2021
Copy link
Owner

@ironfede ironfede left a comment

Choose a reason for hiding this comment

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

Thank you @steffi3011 for your interest in OpenMcdf. I think that surely You've found a bug but not in the meaning implied by your pull request. Description of parameter offset is simply wrong (I'm going to fix it) because it's a refuse from some legacy version where it was publicly exposed by the api. Now, offset is the receiving buffer offset, not the stream one and when you calculate the number of bytes to read you have to choose the minimum between count and buffer dimension minus offset inside it.
So, summing up, the correct way to do this should be
count = (int)Math.Min((long)(buffer.Length - offset), (long)count);
I'm going to commit this change even if the fact that offset should not be reachable from the public api mitigates its impact.
I hope to have been clear enough in this explanation, anyhow many thanks to you for your advice.
Federico

@ironfede ironfede mentioned this pull request Feb 9, 2021
@ironfede ironfede closed this Apr 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants