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: possible overflow when using non-standard Dataset size #87

Merged
merged 1 commit into from
Jun 29, 2019

Conversation

tevador
Copy link
Owner

@tevador tevador commented Jun 25, 2019

Overflow is possible here if Dataset size is over 4 GiB.

cache->datasetInit(cache, dataset->memory + startItem * randomx::CacheLineSize, startItem, startItem + itemCount);

Also added a static_assert to limit Dataset to a maximum of 16 GiB.

@@ -43,6 +43,7 @@ namespace randomx {
static_assert((RANDOMX_DATASET_BASE_SIZE & (RANDOMX_DATASET_BASE_SIZE - 1)) == 0, "RANDOMX_DATASET_BASE_SIZE must be a power of 2.");
static_assert(RANDOMX_DATASET_BASE_SIZE <= 4294967296ULL, "RANDOMX_DATASET_BASE_SIZE must not exceed 4294967296.");
static_assert(RANDOMX_DATASET_EXTRA_SIZE % 64 == 0, "RANDOMX_DATASET_EXTRA_SIZE must be divisible by 64.");
static_assert((uint64_t)RANDOMX_DATASET_BASE_SIZE + RANDOMX_DATASET_EXTRA_SIZE <= 17179869184, "Dataset size must not exceed 16 GiB.");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is 16GB an arbitrary limit? Why do we need this limit?

Copy link
Owner Author

Choose a reason for hiding this comment

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

The theoretical maximum is 256 GiB, because at that point dataset indexes will start to overflow.

But the maximum portion of the Dataset that is used by a program is 4 GiB, so it doesn't make much sense to have a very large Dataset. 16 GiB is an arbitrary limit to make sure RandomX is not used with broken parameters.

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