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

Sf379 qsort crash #189

Closed
wants to merge 4 commits into from
Closed

Sf379 qsort crash #189

wants to merge 4 commits into from

Conversation

d-lamb
Copy link
Member

@d-lamb d-lamb commented Jun 27, 2016

No description provided.

If a user mistakenly passed a perl scalar argument to qsort and
its ilk, the SV would be mistaken for a piddle and passed to
the underlying generic_qsort etc routines. Not surprisingly,
those routine's array index tricks would cause a segfault. This
commit causes qsort, qsorti, qsortvec, and qsortveci to barf
if an argument is passed with incorrect dimensions.

Some care is taken to allow trivial sorts.

2 tests for each routine have been added, and some existing
tests labeled.
If a user mistakenly passed a perl scalar argument to qsort and
its ilk, the SV would be mistaken for a piddle and passed to
the underlying generic_qsort etc routines. Not surprisingly,
those routine's array index tricks would cause a segfault. This
commit causes qsort, qsorti, qsortvec, and qsortveci to barf
if an argument is passed with incorrect dimensions.

Some care is taken to allow trivial sorts.

2 tests for each routine have been added, and some existing
tests labeled.
Hopefully this will compile on Strawberry. Not sure why the original
worked OK on my Mac w/ clang.
@d-lamb
Copy link
Member Author

d-lamb commented Jun 27, 2016

Probably. I didn't realize until after I pushed the first commit that that branch was 19 commits behind master--I rebased master, then had to manually resolve the second commit. Turned into a mess.

@mohawk2
Copy link
Member

mohawk2 commented Jun 27, 2016

Please do rebase. I had a total nightmare picking through Chris's LLDF commits to make sure I'd got them all, because of all the multi-way merges. It's worth it!

@coveralls
Copy link

coveralls commented Jun 27, 2016

Coverage Status

Coverage remained the same at 62.696% when pulling b0d0a4c on sf379-qsort-crash into d12d7b1 on master.

@d-lamb
Copy link
Member Author

d-lamb commented Jun 28, 2016

OK, I have the two "content" commits cherry-picked into a new clean branch, which will be rebased onto master without the double-commit and the merge commit. thanks for the heads-up that it got really ugly!

@wchristian wchristian closed this Jun 28, 2016
@wchristian wchristian deleted the sf379-qsort-crash branch June 28, 2016 04:00
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.

None yet

4 participants