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

perfmon_collection_t accessed after destruction. #1497

Closed
srh opened this issue Sep 30, 2013 · 5 comments
Closed

perfmon_collection_t accessed after destruction. #1497

srh opened this issue Sep 30, 2013 · 5 comments
Assignees
Milestone

Comments

@srh
Copy link
Contributor

srh commented Sep 30, 2013

Specifically, its children access it after destruction.

This can be seen clearly in commit 48ed6fd (running the polyglot tests, for example), which adds a field to perfmon_collection_t to help exhibit the bug.

This was uncovered after noticing the intrusive_list_t destructor implementation, with its commented out assertion, which make this kind of bug seem very likely. A perfmon_collection_t is accessed after destruction by its constituents trying to remove themselves.

    ~intrusive_list_t() {
        //rassert(empty());
    }
@ghost ghost assigned srh Sep 30, 2013
@srh
Copy link
Contributor Author

srh commented Sep 30, 2013

I am working on this in branch sam_1497, which is based off of v1.10.x.

@srh
Copy link
Contributor Author

srh commented Sep 30, 2013

This seems to be the result of the perfmon_collection field being declared after the cache field in internal_disk_backed_queue_t.

@srh
Copy link
Contributor Author

srh commented Sep 30, 2013

While the code is bad, this bug does not result in bad behavior or real unreliable behavior in practice, because the perfmon_collection_t is part of an internal_disk_backed_queue_t that is still in the process of being destructed -- so its memory is sitting around unused, and its destructors don't do anything interesting.

I'm moving this issue to 1.11.

@srh
Copy link
Contributor Author

srh commented Sep 30, 2013

This is currently in code review 964.

@srh
Copy link
Contributor Author

srh commented Sep 30, 2013

This is merged to next as of 7129359.

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

No branches or pull requests

1 participant