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

Make map and grep preserve their args lists. #19403

Closed
wants to merge 1 commit into from

Conversation

FGasper
Copy link
Contributor

@FGasper FGasper commented Feb 8, 2022

Issue #19340: This prevents an obscure segfault.

@Leont
Copy link
Contributor

Leont commented Feb 8, 2022

I don't think leaking really is the best solution we can offer here.

@FGasper
Copy link
Contributor Author

FGasper commented Feb 8, 2022

@Leont Given how small ARGV usually is I didn’t think it unreasonable, though obviously preserving the cleanup would be best for all. Do you have any suggestions of where to start? Some way to prevent the SAVEFREESV from nextargv from conflicting with the leave-time free of the saved $_?

Issue Perl#19340: Previously if you mapped/grepped over an array and mutated
the array within the map/grep, a segfault would happen. This fixes that
by bumping the map/grep args' reference count at the start of the map/grep,
then enqueueing those args for a refcount decrement at the end.
@FGasper FGasper force-pushed the issue_19340_argv_segfault branch from 9bc9e83 to 2ad6fe1 Compare February 9, 2022 01:26
@FGasper FGasper changed the title Prevent @ARGV and <> from conflicting. Make map and grep preserve their args lists. Feb 9, 2022
@FGasper FGasper requested review from atoomic and Leont February 9, 2022 01:37
@Leont Leont requested review from iabyn and tonycoz February 9, 2022 02:19
@Leont
Copy link
Contributor

Leont commented Feb 9, 2022

This should have been a new PR, now my first comment doesn't make sense anymore.

I don't think this is going to fly either. This is one of the more difficult problems that we have, please don't try to quick-fix it.

@FGasper
Copy link
Contributor Author

FGasper commented Feb 9, 2022

@Leont I don't mean to oversimplify, certainly. But can you give a bit more detail as to what else needs to happen? This approach doesn't incur leakage and does solve both of the reproductions in the issue thread.

I don't doubt that there are bigger problems in play; I'd just like to have a better sense of those. Thank you for your time.

@xenu
Copy link
Member

xenu commented Feb 9, 2022

This is a good starting point: #12315 (comment)

@xenu
Copy link
Member

xenu commented Feb 9, 2022

Also see this meta ticket for an incomplete list of related tickets (the dependsOn section).

@FGasper
Copy link
Contributor Author

FGasper commented Feb 9, 2022

@xenu: Thank you for the clarification and broader context.

I would still think that fixing 30% of the cases (i.e., map and grep) is better than solving none of them. To @demerphq’s point, though, this really is a “stop hitting yourself” thing; there seems to be no reason why normal development would result in such code being written, let alone being put into production.

Anyhow, I‘ll close this.

@FGasper FGasper closed this Feb 9, 2022
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

Successfully merging this pull request may close these issues.

3 participants