Skip to content

Do not adjust externally allocated segments #7171

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

Merged
merged 1 commit into from
Dec 24, 2022
Merged

Do not adjust externally allocated segments #7171

merged 1 commit into from
Dec 24, 2022

Conversation

PaulWessel
Copy link
Member

See this pygmt post for details. The problem was that ternary converts the incoming a,b,c[,z] to x,y[,z] and in the process tries to remove that extra column. This ends up in gmtio_adjust_segment which did not check the allocation mode of the vectors it was trying to free, thus trying to free memory not belonging to GMT. So this is an API bug in a function rarely called.

Description of proposed changes

Check allocation status of the affected vectors and only free if allocated by GMT, else set to NULL.

Question: If you examine df after the plot has finished you will see that the content has changed since ternary changes it. If you feel that is not right and should be considered read-only memory then your ternary pyGMT wrapper needs to pass GMT_IS_DUPLICATE instead of GMT_IS_REFERENCE. You can make a decision on that but at least the SEGV bug in the API has been fixed, I hope.

See this pygmt post for details.  The problem was that the gmtio_adjust_segment did not check the allocation mode of the vectors it was trying to free, thus trying to free memory not belonging to GMT.  ternary converts the incoming a,b,c[,z] to x,y[,z] in in the process tries to remove that extra column.
@PaulWessel PaulWessel added the bug Something isn't working label Dec 24, 2022
@PaulWessel PaulWessel added this to the 6.5.0 milestone Dec 24, 2022
@PaulWessel PaulWessel requested a review from seisman December 24, 2022 11:47
@PaulWessel PaulWessel self-assigned this Dec 24, 2022
@seisman
Copy link
Member

seisman commented Dec 24, 2022

Question: If you examine df after the plot has finished you will see that the content has changed since ternary changes it. If you feel that is not right and should be considered read-only memory then your ternary pyGMT wrapper needs to pass GMT_IS_DUPLICATE instead of GMT_IS_REFERENCE. You can make a decision on that but at least the SEGV bug in the API has been fixed, I hope.

Yes, I can confirm that this PR fixes the SEGV bug.

As what you said, the variable df changes from:

      a      b      c     d
0  0.16  0.331  0.509   9.0
1  0.22  0.625  0.158  39.0
2  0.09  0.180  0.732   2.0
3  0.66  0.258  0.078  12.0

to

          a         b     c     d
0  0.325500  0.286654   9.0   9.0
1  0.530907  0.539647  39.0  39.0
2  0.179641  0.155573   2.0   2.0
3  0.792169  0.224332  12.0  12.0

which is unexpected.

@PaulWessel PaulWessel merged commit cea79ba into master Dec 24, 2022
@PaulWessel PaulWessel deleted the no-ext-free branch December 24, 2022 14:07
@PaulWessel
Copy link
Member Author

Yes, I do agree this is unexpected. Given that nobody will plot 2 Tb of points in a ternary (we hope) it is probably OK if ternary inside does the duplication to avoid surprises on the other end. But I think this is possibly also true for other situations. Many grid operators modify the input grid rather than duplicate, for instance.

@seisman
Copy link
Member

seisman commented Dec 24, 2022

Given that nobody will plot 2 Tb of points in a ternary (we hope) it is probably OK if ternary inside does the duplication to avoid surprises on the other end.

It would be better if ternary can do the duplication on the C side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants