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

Properly mark a deleted typemap entry #27568

Merged
merged 1 commit into from
Jun 16, 2018
Merged

Properly mark a deleted typemap entry #27568

merged 1 commit into from
Jun 16, 2018

Conversation

Keno
Copy link
Member

@Keno Keno commented Jun 14, 2018

This issue possibly fixes #24951 (or at least the test case by iamed2).
We believe the original code here meant to say either:

((jl_typemap_entry_t*)v)->min_world = ((jl_typemap_entry_t*)v)->max_world + 1;

or

((jl_typemap_entry_t*)v)->max_world = ((jl_typemap_entry_t*)v)->min_world - 1;

i.e. set the range of applicable worlds to be empty. What happened instead
was that the given typemap entry that was supposed to be deleted became valid
for one particular world and that world only. Thus any code running in that
particular world may try to access the deleted typemap entry (or add a backedge
to it), causing either incorrect behavior or the assertion failure noted
in the issue. One additional complication is that these world ages are being
deserialized, i.e. they may be larger than the currently possible max world age.
This makes this slightly more likely to happen, since the current process
may work its way up to that world age and exectue some code.

In any case, there's not much value to keeping around the deserialized max or min
world, so just mark them as 1:0, as we do for other deleted entries.

@vtjnash is gonna try working on a test case for this, but it's very tricky to trigger.

@vtjnash
Copy link
Member

vtjnash commented Jun 14, 2018

I wasn't able to trigger this with a test, but I added some assertion code that should detect earlier if it happens organically.

This issue possibly fixes #24951 (or at least the test case by iamed2).
We believe the original code here meant to say either:

    ((jl_typemap_entry_t*)v)->min_world = ((jl_typemap_entry_t*)v)->max_world + 1;

or

    ((jl_typemap_entry_t*)v)->max_world = ((jl_typemap_entry_t*)v)->min_world - 1;

i.e. set the range of applicable worlds to be empty. What happened instead
was that the given typemap entry that was supposed to be deleted became valid
for one particular world and that world only. Thus any code running in that
particular world may try to access the deleted typemap entry (or add a backedge
to it), causing either incorrect behavior or the assertion failure noted
in the issue. One additional complication is that these world ages are being
deserialized, i.e. they may be larger than the currently possible max world age.
This makes this slightly more likely to happen, since the current process
may work its way up to that world age and exectue some code.

In any case, there's not much value to keeping around the deserialized max or min
world, so just mark them as [1:0], as we do for other deleted entries.

Co-authored-by: Jameson Nash <vtjnash@gmail.com>
@ararslan ararslan added the bugfix This change fixes an existing bug label Jun 15, 2018
@ararslan
Copy link
Member

Once this is merged I can try to put together a 0.6.4 release in short order.

@Keno
Copy link
Member Author

Keno commented Jun 15, 2018

@staticfloat Can you check whether this fixes your issue as well?

@Keno Keno merged commit d9b10f0 into master Jun 16, 2018
@ararslan ararslan deleted the kf/jn/24951 branch June 16, 2018 03:46
ararslan pushed a commit that referenced this pull request Jun 17, 2018
This issue possibly fixes #24951 (or at least the test case by iamed2).
We believe the original code here meant to say either:

    ((jl_typemap_entry_t*)v)->min_world = ((jl_typemap_entry_t*)v)->max_world + 1;

or

    ((jl_typemap_entry_t*)v)->max_world = ((jl_typemap_entry_t*)v)->min_world - 1;

i.e. set the range of applicable worlds to be empty. What happened instead
was that the given typemap entry that was supposed to be deleted became valid
for one particular world and that world only. Thus any code running in that
particular world may try to access the deleted typemap entry (or add a backedge
to it), causing either incorrect behavior or the assertion failure noted
in the issue. One additional complication is that these world ages are being
deserialized, i.e. they may be larger than the currently possible max world age.
This makes this slightly more likely to happen, since the current process
may work its way up to that world age and exectue some code.

In any case, there's not much value to keeping around the deserialized max or min
world, so just mark them as [1:0], as we do for other deleted entries.

Co-authored-by: Jameson Nash <vtjnash@gmail.com>

---

NOTE: This backported commit EXCLUDES additional assertions made by
vtjnash.

(Cherry-picked from commit d9b10f0)
@iamed2 iamed2 mentioned this pull request Jun 26, 2018
3 tasks
Keno added a commit that referenced this pull request Jul 20, 2018
This assertion was added in #27568. However from my understanding
the first part of the assertion is wrong, since that PR added
logic to mark deleted typemap entries as min_world 0, max_world 1.
It seems valid for a typemap entry to be deleted, even if the referenced
(while the reverse does not seem valid).
Keno added a commit that referenced this pull request Jul 20, 2018
This assertion was added in #27568. However from my understanding
the first part of the assertion is wrong, since that PR added
logic to mark deleted typemap entries as min_world 0, max_world 1.
It seems valid for a typemap entry to be deleted, even if the referenced
(while the reverse does not seem valid).

Fixes #28213
Keno added a commit that referenced this pull request Jul 21, 2018
This assertion was added in #27568. However from my understanding
the first part of the assertion is wrong, since that PR added
logic to mark deleted typemap entries as min_world 0, max_world 1.
It seems valid for a typemap entry to be deleted, even if the referenced
(while the reverse does not seem valid).

Fixes #28213
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MethodError no method matching convert(::Type{AssertionError}, ::String)
3 participants