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

Fix unsafe memory access #376

Closed
wants to merge 1 commit into from
Closed

Fix unsafe memory access #376

wants to merge 1 commit into from

Conversation

shargon
Copy link
Member

@shargon shargon commented Sep 13, 2018

Without checking the length, is possible to read arbitrary memory

  • Should be merged to master

Is possible to read arbitrary memory
@erikzhang
Copy link
Member

erikzhang commented Sep 13, 2018

These unsafe methods are designed for performance, so some checks are omitted. They require developers to use them carefully and are not public.

@shargon
Copy link
Member Author

shargon commented Sep 13, 2018

I think that the length should be checked, is easy to forget the previous check, for example in this:

get
{
byte[] buffer = LoadStoredData("Version");
if (buffer == null) return new Version(0, 0);
int major = buffer.ToInt32(0);
int minor = buffer.ToInt32(4);
int build = buffer.ToInt32(8);
int revision = buffer.ToInt32(12);
return new Version(major, minor, build, revision);
}

@erikzhang
Copy link
Member

erikzhang commented Sep 13, 2018

We need these high performance methods in some places.

For example:

uint k = array.ToUInt32(i);

We can find out where the methods are misused and then use BitConverter.ToXXX() instead.

@shargon shargon mentioned this pull request Sep 14, 2018
@shargon
Copy link
Member Author

shargon commented Sep 14, 2018

I will search and fix where the length is not checked, and close this issue

@shargon shargon mentioned this pull request Sep 14, 2018
shargon added a commit that referenced this pull request Sep 14, 2018
@shargon shargon mentioned this pull request Sep 14, 2018
@shargon
Copy link
Member Author

shargon commented Sep 14, 2018

I think that all is fixed now, i close it

@shargon shargon closed this Sep 14, 2018
@shargon shargon deleted the fix-memory-access branch September 14, 2018 08:24
Thacryba pushed a commit to simplitech/neo that referenced this pull request Feb 17, 2020
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.

None yet

2 participants