-
Notifications
You must be signed in to change notification settings - Fork 103
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
Fixes unicode to string conversion in check_http_expect in Py3 #102
Conversation
meinheld/server/server.c
Outdated
@@ -857,7 +857,7 @@ check_http_expect(client_t *client) | |||
///TODO CHECK | |||
c = PyDict_GetItemString(req->environ, "HTTP_EXPECT"); | |||
if (c) { | |||
val = PyBytes_AS_STRING(c); | |||
val = NATIVE_ASSTRING(c); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use PyUnicode_AsUTF8
.
NATIVE_ASSTRING has serias bug (use-after-free).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That function is incompatible with python2. Should #define NATIVE_ASSTRING as_str
be changed to #define NATIVE_ASSTRING PyUnicode_AsUTF8
for just PY3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That apparently works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That function is incompatible with python2. Should
#define NATIVE_ASSTRING as_str
be changed to#define NATIVE_ASSTRING PyUnicode_AsUTF8
for just PY3?
No.
NATIVE_ASSTRING used latin1. It is PEP 333 requirement.
PyUnicode_AsUTF8 is safe here because we only used ASCII here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, NATIVE_ASSTRING is not used anywhere. I want to just remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That function is incompatible with python2. Should
#define NATIVE_ASSTRING as_str
be changed to#define NATIVE_ASSTRING PyUnicode_AsUTF8
for just PY3?No.
NATIVE_ASSTRING used latin1. It is PEP 333 requirement.
PyUnicode_AsUTF8 is safe here because we only used ASCII here.
Ah, NATIVE_ASSTRING is not used anywhere. I want to just remove it.
I think removing NATIVE_ASSTRING
makes sense; but in either case we need two different functions at line 860 in server.c
, for Py2 vs Py3. In Py3 we need to call PyUnicode_AsUTF8
; but, as far as I can tell, in Py2, we still need to use PyBytes_AS_STRING
, because as you can see when I tried just replacing PyBytes_AS_STRING
with PyUnicode_AsUTF8
on just line 860 in server.c
(commit 524945
), it fails to compile on Py2.7, because PyUnicode_AsUTF8
doesn't exist in Py2.7.
If NATIVE_ASSTRING
is unused anywhere else, it could probably be removed; but in either case we need to define a macro that points to PyBytes_AS_STRING
on Py2 and PyUnicode_AsUTF8
on Py3, for use in the check_http_expect
function. Do you have a preference for what to call this macro? Maybe PyStr_ASSTRING
?
Alternatively, I guess we can do the Py2 vs Py3 check in the function itself...
Do you have a preference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, I guess we can do the Py2 vs Py3 check in the function itself...
I prefer this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
See this SO question for symptom.