hnswalg.h: cap M to 100000 #484
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This patch works around issue nmslib#467, also referenced as CVE-2023-37365, by implementing Yury Malkov's suggestion about capping the M value, coding the maximum number of outgoing connections in the graph, to a reasonable enough value of the order of 100000. For the record, the documentation indicates reasonable values for M range from 2 to 100, which are well within the cap; see ALGO_PARAMS.md. The reproducer shown in issue nmslib#467 doesn't trigger the double free condition anymore after this change is applied, but completes successfully, although with the below warning popping up on purpose: warning: M parameter exceeds 100000 which may lead to adverse effects. Cap to 100000 will be applied for the rest of the processing. Signed-off-by: Étienne Mollier <[email protected]>
|
Thank you for the PR! |
per comment in merge request discussion.
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch works around issue #467, also referenced as CVE-2023-37365, by implementing Yury Malkov's suggestion about capping the M value, coding the maximum number of outgoing connections in the graph, to a reasonable enough value of the order of 100000. For the record, the documentation indicates reasonable values for M range from 2 to 100, which are well within the cap; see ALGO_PARAMS.md.
The reproducer shown in issue #467 doesn't trigger the double free condition anymore after this change is applied, but completes successfully, although with the below warning popping up on purpose: