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

[ML] Check accesses in bounds when clearing recycled models #79

Merged
merged 2 commits into from May 1, 2018

Conversation

Projects
None yet
4 participants
@tveasey
Copy link
Contributor

commented May 1, 2018

Add failsafes to ensure that we never try and reset state for identifiers outside vector bounds. This shouldn't happen, but this is the likely cause of #76 and there are no side effects from checking and issuing an error in such cases. Investigating and fixing the root cause of #76 will be the subject of separate PR.

@tveasey tveasey requested a review from edsavage May 1, 2018

@tveasey tveasey changed the title [ML] Check range when clearing recycled models [ML] Check range in bounds when clearing recycled models May 1, 2018

m_PersonAttributeBucketCounts[cid] = *m_NewPersonBucketCounts;
}
} else {
LOG_ERROR("Recycled attribute identifier '"

This comment has been minimized.

Copy link
@droberts195

droberts195 May 1, 2018

Contributor

There's a missing << after the LOG_ERROR(.

Also, have you run clang-format on this file? CI is set to fail now if the formatting isn't what clang-format would choose.

@droberts195

This comment has been minimized.

Copy link
Contributor

commented May 1, 2018

I think it would be good to make the same fix in 5.6.10 as well, since that was where the problem was reported, and some people will be using 5.6 until early next year.

@edsavage
Copy link
Contributor

left a comment

LGTM, just one minor change requested

if (pid < m_PersonBucketCounts.size()) {
m_PersonBucketCounts[pid] = 0.0;
} else {
LOG_ERROR("Recycled person identifier '"

This comment has been minimized.

Copy link
@edsavage

edsavage May 1, 2018

Contributor

Missing '<<' at the start of the error message

@tveasey

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2018

Agreed Dave, I was planning to make the necessary changes in that repo too.

@tveasey tveasey merged commit a1a7287 into elastic:master May 1, 2018

1 check passed

CLA Commit author is a member of Elasticsearch
Details

tveasey added a commit that referenced this pull request May 1, 2018

tveasey added a commit that referenced this pull request May 1, 2018

@tveasey tveasey changed the title [ML] Check range in bounds when clearing recycled models [ML] Check accesses in bounds when clearing recycled models May 2, 2018

@lcawl lcawl added the >bug label Jun 8, 2018

@tveasey tveasey deleted the tveasey:bug/sigsegv-update-recycled-models branch Dec 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.