clone mg_ptr for magic #25
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Handle mg_ptr to avoid to share it with other PVs.
|
@HaraldJoerg Thank you for reporting! Clone 0.42 should hit your friendly CPAN mirror any moment now :) |
atoomic
commented
Jul 19, 2019
| Newxz(mg_ptr, mg->mg_len+1, char); /* add +1 for the NULL at the end? */ | ||
| Copy(mg->mg_ptr, mg_ptr, mg->mg_len, char); | ||
| } | ||
| } else if (mg->mg_len == HEf_SVKEY) { |
There was a problem hiding this comment.
I realized that we have no unit tests to cover this part...
atoomic
commented
Jul 19, 2019
| /* let's share the SV for now */ | ||
| SvREFCNT_inc((SV*)mg->mg_ptr); | ||
| /* maybe we also want to clone the SV... */ | ||
| //if (mg_ptr) mg->mg_ptr = (char*) sv_clone((SV*)mg->mg_ptr, hseen, -1); |
There was a problem hiding this comment.
maybe this is what we should prefer doing, the current solution above just do one SvRefCnt++
|
Hey @atoomic! Looks like we broke Geo::Address::Formatter 😱 https://rt.cpan.org/Ticket/Display.html?id=130195 Could you maybe take a look and see if we can fix it? If you have the time, of course! |
|
interesting, I will have a look at this
thanks for the update
…On Mon, Jul 29, 2019 at 12:59 PM Breno G. de Oliveira < ***@***.***> wrote:
Hey @atoomic <https://github.com/atoomic>! Looks like we broke
Geo::Address::Formatter 😱
https://rt.cpan.org/Ticket/Display.html?id=130195
Could you maybe take a look and see if we can fix it? If you have the
time, of course!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#25?email_source=notifications&email_token=AADC6IQ2IJGN7AKBI5ZAUVTQB44XJA5CNFSM4IEKEY32YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3BVYSI#issuecomment-516119625>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADC6IR4DKHRG4SX3V3T2CLQB44XJANCNFSM4IEKEY3Q>
.
|
|
This is going to be fixed via #26
Not sure if I can do a release of it, @garu can you review and publish a
release?
thanks
nicolas
…On Mon, Jul 29, 2019 at 12:59 PM Breno G. de Oliveira < ***@***.***> wrote:
Hey @atoomic <https://github.com/atoomic>! Looks like we broke
Geo::Address::Formatter 😱
https://rt.cpan.org/Ticket/Display.html?id=130195
Could you maybe take a look and see if we can fix it? If you have the
time, of course!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#25?email_source=notifications&email_token=AADC6IQ2IJGN7AKBI5ZAUVTQB44XJA5CNFSM4IEKEY32YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3BVYSI#issuecomment-516119625>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADC6IR4DKHRG4SX3V3T2CLQB44XJANCNFSM4IEKEY3Q>
.
|
|
While you're at it: I stumbled over another segfault problem when a structure happened to contain a DBI database handle. On Linux, the demo attached dies with Clone 0.41 and Clone 0.42, while it seems to work on Windows. Cloning database handles is, of course, veeery suspicious (and should not have happened in my code in the first place), but some sort of safeguarding in Clone would be nice. |
|
Any chance you could report that as a GitHub ticket, with what would be
nice a simple script to reproduce the error?
Any chance this is related to #17
thanks in advance
nicolas
…On Mon, Jul 29, 2019 at 1:36 PM Harald Jörg ***@***.***> wrote:
While you're at it: I stumbled over another segfault problem when a
structure happened to contain a DBI database handle. On Linux, the demo
attached dies with Clone 0.41 and Clone 0.42, while it seems to work on
Windows.
Cloning database handles is, of course, veeery suspicious (and should not
have happened in my code in the first place), but some sort of safeguarding
in Clone would be nice.
dbhclone.txt <https://github.com/garu/Clone/files/3444009/dbhclone.txt>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#25?email_source=notifications&email_token=AADC6IXBXC2U7LY6UAIWDQLQB5BCPA5CNFSM4IEKEY32YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3BY2GQ#issuecomment-516132122>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADC6ISOHQUTNQECPZD3TGTQB5BCPANCNFSM4IEKEY3Q>
.
|
|
Sure, will report an issue. The relation to #17 is probably just what's given there as a root cause: "Database handles can't be cloned". |
|
ok I realized I've permissions to publish Clone,
I've just uploaded an updated version of Clone 0.43 to CPAN,
it should be available in the next minutes.
Please confirm that this is fixing your issue.
sincerely
nicolas
…On Mon, Jul 29, 2019 at 1:35 PM Nicolas R. ***@***.***> wrote:
This is going to be fixed via #26
Not sure if I can do a release of it, @garu can you review and publish a
release?
thanks
nicolas
On Mon, Jul 29, 2019 at 12:59 PM Breno G. de Oliveira <
***@***.***> wrote:
> Hey @atoomic <https://github.com/atoomic>! Looks like we broke
> Geo::Address::Formatter 😱
>
> https://rt.cpan.org/Ticket/Display.html?id=130195
>
> Could you maybe take a look and see if we can fix it? If you have the
> time, of course!
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#25?email_source=notifications&email_token=AADC6IQ2IJGN7AKBI5ZAUVTQB44XJA5CNFSM4IEKEY32YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3BVYSI#issuecomment-516119625>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AADC6IR4DKHRG4SX3V3T2CLQB44XJANCNFSM4IEKEY3Q>
> .
>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Handle mg_ptr to avoid to share it with other PVs.