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

fix for rt-43537: undefined values passed to the multi functions no long... #5

Merged
merged 2 commits into from Jan 22, 2015

Conversation

Projects
None yet
2 participants
@mcmillhj
Contributor

mcmillhj commented Jan 22, 2015

This patch fixes rt-43537:

Calls to set_multi that contained an undefined value caused segmentation faults. The root cause of this was the de-referencing of a call to av_fetch() which returned a NULL pointer.

To fix this a new function, safe_av_fetch(), was added to Fast.xs that checks the result of av_fetch for a NULL pointer or a pointer that does not pass an SvOK() check. If such a value is encountered safe_av_fetch() croaks.

A new test t/malformed.t was added to test this new functionality. It adds three values to the server, then submits a series calls to set_mutli() with invalid arguments (in varying positions). It verifies that none of these calls changes the values of the keys on the server.

@kroki

This comment has been minimized.

Show comment
Hide comment
@kroki

kroki Jan 22, 2015

Seems like you forgot to pass aTHX_ to safe_av_fetch() per our discussion.

kroki commented on Fast.xs in 60e8439 Jan 22, 2015

Seems like you forgot to pass aTHX_ to safe_av_fetch() per our discussion.

@mcmillhj

This comment has been minimized.

Show comment
Hide comment
@mcmillhj

mcmillhj Jan 22, 2015

Contributor

My mistake, I even had that on a list of things to do and I still forgot. Is aTHX_ a requirement? I am surprised it built properly if it is.

Contributor

mcmillhj commented Jan 22, 2015

My mistake, I even had that on a list of things to do and I still forgot. Is aTHX_ a requirement? I am surprised it built properly if it is.

@kroki

This comment has been minimized.

Show comment
Hide comment
@kroki

kroki Jan 22, 2015

Contributor

aTHX_ is an argument (hence "a", underscore suggests a comma) for a pTHX_ parameter (hence "p"). You need aTHX_ because you have pTHX_, and you need pTHX_ because when Perl is built with the support for multi-threading av_fetch() (and friends) is a macro that expands to a function call with an extra argument of thread context. I.e. pTHX_ expands to "ThreadContextType *ctx," (actual names are different), aTHX_ expands to "ctx,", and av_fetch(a,b,c) expands to av_fetch(ctx,a,b,c), thus passing around current thread context "ctx". Before the commit 13c9216 we haven't defined PERL_NO_GET_CONTEXT and so didn't have pTHX_/aTHX_, but then av_fetch(a,b,c) had expanded to av_fetch(get_current_ctx(),a,b,c) and there were too many get_current_ctx() calls, which bothered some.

Presumably on OSX you have single-threaded Perl, hence pTHX_/aTHX_ expand to empty strings, and av_fetch(a,b,c) expands to av_fetch(global_ctx_var,a,b,c), and so you don't have compilation problem. But on Debian you had an error yourself ;).

Contributor

kroki commented Jan 22, 2015

aTHX_ is an argument (hence "a", underscore suggests a comma) for a pTHX_ parameter (hence "p"). You need aTHX_ because you have pTHX_, and you need pTHX_ because when Perl is built with the support for multi-threading av_fetch() (and friends) is a macro that expands to a function call with an extra argument of thread context. I.e. pTHX_ expands to "ThreadContextType *ctx," (actual names are different), aTHX_ expands to "ctx,", and av_fetch(a,b,c) expands to av_fetch(ctx,a,b,c), thus passing around current thread context "ctx". Before the commit 13c9216 we haven't defined PERL_NO_GET_CONTEXT and so didn't have pTHX_/aTHX_, but then av_fetch(a,b,c) had expanded to av_fetch(get_current_ctx(),a,b,c) and there were too many get_current_ctx() calls, which bothered some.

Presumably on OSX you have single-threaded Perl, hence pTHX_/aTHX_ expand to empty strings, and av_fetch(a,b,c) expands to av_fetch(global_ctx_var,a,b,c), and so you don't have compilation problem. But on Debian you had an error yourself ;).

kroki added a commit that referenced this pull request Jan 22, 2015

Merge pull request #5 from mcmillhj/bug/rt-43537
fix for rt-43537: undefined values passed to the multi functions no longer cause segmentation faults

@kroki kroki merged commit c96cf90 into JRaspass:master Jan 22, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment