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

Avoid array out of bounds read in friend saving. #346

Merged
merged 1 commit into from
Dec 20, 2016

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Dec 19, 2016

Fixes #345.


This change is Reviewable

@iphydf iphydf added this to the v0.1.2 milestone Dec 19, 2016
@Diadlo
Copy link

Diadlo commented Dec 20, 2016

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


toxcore/Messenger.c, line 2732 at r1 (raw file):

            if (temp.status < 3) {
                memcpy(temp.info, m->friendlist[i].info,
                       MIN(m->friendlist[i].info_size, MIN(SAVED_FRIEND_REQUEST_SIZE, MAX_FRIEND_REQUEST_DATA_SIZE)));

Why not precalculate MIN(SAVED_FRIEND_REQUEST_SIZE, MAX_FRIEND_REQUEST_DATA_SIZE)?
Maybe extract result of MIN(..MIN()) into separate variable? IMO it will make reading easier.


Comments from Reviewable

@robinlinden
Copy link
Member

:lgtm_strong:


Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Dec 20, 2016

:lgtm_strong:


Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


toxcore/Messenger.c, line 2732 at r1 (raw file):

Why not precalculate MIN(SAVED_FRIEND_REQUEST_SIZE, MAX_FRIEND_REQUEST_DATA_SIZE)?

MIN(SAVED_FRIEND_REQUEST_SIZE, MAX_FRIEND_REQUEST_DATA_SIZE) should be precalculated by compiler at compile-time, it's a const expression.


Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Dec 20, 2016

@iphydf just curious, what's aioobe, the branch name? Array Index Out Off Bounds Error?

@Diadlo
Copy link

Diadlo commented Dec 20, 2016

:lgtm_strong:


Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


toxcore/Messenger.c, line 2732 at r1 (raw file):

Previously, nurupo wrote…

Why not precalculate MIN(SAVED_FRIEND_REQUEST_SIZE, MAX_FRIEND_REQUEST_DATA_SIZE)?

MIN(SAVED_FRIEND_REQUEST_SIZE, MAX_FRIEND_REQUEST_DATA_SIZE) should be precalculated by compiler at compile-time, it's a const expression.

Thanks for explanation


Comments from Reviewable

@iphydf
Copy link
Member Author

iphydf commented Dec 20, 2016

Yes: https://docs.oracle.com/javase/7/docs/api/java/lang/ArrayIndexOutOfBoundsException.html. There was no exception in C, and ASAN can't catch it, but in Java there would have been an AIOOBE.


Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


toxcore/Messenger.c, line 2732 at r1 (raw file):

Previously, Diadlo (Polshakov Dmitry) wrote…

Thanks for explanation

Moved it to a variable.


Comments from Reviewable

@iphydf iphydf merged commit 5237844 into TokTok:master Dec 20, 2016
@iphydf iphydf deleted the aioobe branch December 20, 2016 14:37
This pull request was closed.
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.

4 participants