Created attachment 613627 [details] Remove the unnecessary range check in pycurl Description of problem: Curl checks the return value of write_callback against the total buffer size and aborts the download cleanly if they don't match. Callbacks can thus easily abort the download eg. with "return -1". However, pycurl layer checks that the return value is in range 0..total_size, and raises "invalid return value for write callback" exception otherwise. Version-Release number of selected component (if applicable): How reproducible: Always Steps to Reproduce: 1. Use a Python writefunction that returns -1. 2. Start a download with pycurl. 3. Actual results: "invalid return value for write callback -1" exception. Expected results: Download fails with "Failed writing body" or similar error. Additional info: A possible workaround is for the callback to return 0 instead of -1, but that 1) wouldn't work if curl sent an empty buffer 2) required to use different values in C and Python callbacks to signal an abort.
Looked at the patch again, and found a small bug. The callback may return PyLong_type, then PyInt_Check(result) fails. But we should try to convert it to int anyway.. - else if (PyInt_Check(result)) { + else if (PyInt_Check(result) || PyLong_Check(result)) {
Any chance you can use 'None' to specify an error?
None is already used as "everything okay", mis-using it would break API.. if (result == Py_None) { ret = total_size; /* None means success */
One more thing. It seems that curl expects a size_t return value. The return value of write_callback and util_write_callback is size_t as well. Returning -1 only would only mean returning the maximum integer value. Yes, if you do this on purpose and then re-type it to int again, then you have -1 again. But I call that dirty.
s/-1/UINT_MAX then. Looking at sources it seems that curl never calls write callback with len==0, so returning 0 to signal abort should work, too. But that unnecessary range check still breaks CURL_WRITEFUNC_PAUSE, so I think it should be removed. /* This is a magic return code for the write callback that, when returned, will signal libcurl to pause receiving on the current transfer. */ #define 0x10000001
Created attachment 658097 [details] Updated version of the previous patch Ok, removing the upper bound check should fix this.
(In reply to comment #5) > that unnecessary range check still breaks CURL_WRITEFUNC_PAUSE, so I think > it should be removed. Zdenek, I updated the patch. Does it look ok to you?
Why do you keep two almost identical code paths? PyInt_AsLong() converts *both* int and long to C long just fine. long PyInt_AsLong(PyObject *io): Will first attempt to cast the object to a PyIntObject, if it is not already one, and then return its value. Also, what's the purpose of trapping negative values? Curl handles *any* size_t value, including ones with MSB set. Removing the upper bound check makes CURL_WRITEFUNC_PAUSE work, but I still can't use -1 to signal an error.
This package has changed ownership in the Fedora Package Database. Reassigning to the new owner of this component.
Created attachment 703018 [details] proposed fix
Using two code paths seems unnecessary. PyInt_AsLong() handles both python int and long types. PyLong_AsLong() has the same return type, so it does not help in any way. Quoting Python C API: > long PyInt_AsLong(PyObject *io) > > Will first attempt to cast the object to a PyIntObject, ...
I am not a python programmer myself, but C99's $6.3.1.3:3 does not guarantee much about the conversion of long to int: "Otherwise, the new type is signed and the value cannot be represented in it; either the result is implementation-defined or an implementation-defined signal is raised." The first part of may patch eliminates the duplicated code, which I thought was your main concern. However, I am not convinced we should introduce a new signed cast to save an additional line of code. Please note that the patch will hopefully go upstream, where we can hardly assume anything about the particular python version, target architecture, etc.
No, I tried to remove the "if (obj_size < 0 || obj_size > total_size)" check, because it is not necessary and it: 1) prevents the python callback to return -1 to signal "error in callback, please abort the download and return". 2) python callback can't return CURL_WRITEFUNC_PAUSE, because it's larger than total_size. Your patch does not address this. I'd prefer to use the original patch with change in comment #1, so both python int and long types are accepted.
Created attachment 703490 [details] a script exercising python-to-C int/long conversions Tested with python 2.4 .. 3.3 on i386 and amd64 machines.
Created attachment 703505 [details] proposed fix V2
Created attachment 703527 [details] a script exercising python-to-C int/long conversions V2 I have fixed a copy/paste error in the checker.
Comment on attachment 703505 [details] proposed fix V2 Ack, thanks!
Created attachment 705949 [details] test_write_abort.py: test returning -1 from write callback
fixed in python-pycurl-7.19.0-15.fc19