Control: reopen 683188
Control: notfixed 683188 subversion/1.7.9-1+nmu1
On Sat, Apr 13, 2013 at 11:23:41AM -0400, Michael Gilbert wrote:
> On Sat, Apr 6, 2013 at 7:40 PM, Michael Gilbert wrote:
> > On Sat, Apr 6, 2013 at 7:11 PM, W. Martin Borgert wrote:
> >> On 2013-04-06 18:24, Michael Gilbert wrote:
> >>> Does SVN_STREAM_CHUNK_SIZE even contain the expected value
> >>> of 102400?
> >>
> >> Yes, 102400 of type 'long'.
> >
> > Then that fix should be fine given that 102400 is within the valid
> > range of integers. Plus line 155 essentially does the same int
> > conversion for the other path through that function, so it's probably
> > an oversight that line 148 does not. I say go ahead and nmu it.
>
> Hi, I've uploaded an nmu fixing this issue. Please see attached patch.
No, it isn't fixing the issue, since there are many other callers of
svn_stream_read() out there, many of which use that constant.
What happens is that the python binding for that function is rejecting
longs as it first calls PyInt_Check(), then PyInt_AsLong(). The first
call rejects the long, end of story.
Please find attached a patch against wheezy's package, tested live on
alioth thanks to Stephen Gran.
Looking at svn blame subversion/bindings/swig/core.i to see where the
regression came from, I noticed the code was rewritten in the meanwhile:
| r1351117 | peters | 2012-06-17 16:05:16 +0000 (Sun, 17 Jun 2012) | 5 lines
|
| * subversion/bindings/swig/core.i
| (python typemap: char *buffer, apr_size_t *len): Accept either a
| PyInt or a PyLong buffer length argument. swig 2.0.5+ handles this
| differently to previous releases.
the diff is:
| --- subversion/bindings/swig/core.i (revision 1351116)
| +++ subversion/bindings/swig/core.i (revision 1351117)
| @@ -351,12 +351,17 @@
| */
| #ifdef SWIGPYTHON
| %typemap(in) (char *buffer, apr_size_t *len) ($*2_type temp) {
| - if (!PyInt_Check($input)) {
| + if (PyLong_Check($input)) {
| + temp = PyLong_AsLong($input);
| + }
| + else if (PyInt_Check($input)) {
| + temp = PyInt_AsLong($input);
| + }
| + else {
| PyErr_SetString(PyExc_TypeError,
| "expecting an integer for the buffer size");
| SWIG_fail;
| }
| - temp = PyInt_AsLong($input);
| if (temp < 0) {
| PyErr_SetString(PyExc_ValueError,
| "buffer size must be a positive integer");
Peter, I'll leave it up to you to choose how to fix the package in sid
and in wheezy. I must say I'm slightly sad to have spent so much time
digging into it while there was already an upstream commit fixing it,
and no comment on the debian bug report(s)… :/ But oh well, happy to
have a fix handy now.
(In case my debdiff is considered, the changelog can be rewritten to
mention the swig behavioral change instead of the long-ish explanation.)
Mraw,
KiBi.
PS: For the curious, below is the relevant change in swig, which
explains why we're getting a long object instead of an int one.
| --- swig2.0-2.0.4/Lib/python/pyprimtypes.swg
| +++ swig2.0-2.0.5/Lib/python/pyprimtypes.swg
| @@ -25,10 +25,20 @@
| }
| }
|
| +/* int */
| +
| +%fragment(SWIG_From_frag(int),"header") {
| +SWIGINTERNINLINE PyObject*
| + SWIG_From_dec(int)(int value)
| +{
| + return PyInt_FromLong((long) value);
| +}
| +}
| +
| /* long */
|
| %fragment(SWIG_From_frag(long),"header") {
| - %define_as(SWIG_From_dec(long), PyInt_FromLong)
| + %define_as(SWIG_From_dec(long), PyLong_FromLong)
| }
|
| %fragment(SWIG_AsVal_frag(long),"header",
| @@ -80,7 +90,7 @@
| SWIG_From_dec(unsigned long)(unsigned long value)
| {
| return (value > LONG_MAX) ?
| - PyLong_FromUnsignedLong(value) :
PyInt_FromLong(%numeric_cast(value,long));
| + PyLong_FromUnsignedLong(value) :
PyLong_FromLong(%numeric_cast(value,long));
| }
| }
|
| @@ -139,7 +149,7 @@
| SWIG_From_dec(long long)(long long value)
| {
| return ((value < LONG_MIN) || (value > LONG_MAX)) ?
| - PyLong_FromLongLong(value) : PyInt_FromLong(%numeric_cast(value,long));
| + PyLong_FromLongLong(value) : PyLong_FromLong(%numeric_cast(value,long));
| }
| }
|
| @@ -193,7 +203,7 @@
| SWIG_From_dec(unsigned long long)(unsigned long long value)
| {
| return (value > LONG_MAX) ?
| - PyLong_FromUnsignedLongLong(value) :
PyInt_FromLong(%numeric_cast(value,long));
| + PyLong_FromUnsignedLongLong(value) :
PyLong_FromLong(%numeric_cast(value,long));
| }
| }
diff -Nru subversion-1.6.17dfsg/debian/changelog subversion-1.6.17dfsg/debian/changelog
--- subversion-1.6.17dfsg/debian/changelog 2013-09-17 10:58:33.000000000 +0000
+++ subversion-1.6.17dfsg/debian/changelog 2013-09-17 10:58:34.000000000 +0000
@@ -1,3 +1,18 @@
+subversion (1.6.17dfsg-4+deb7u3.1) UNRELEASED; urgency=low
+
+ * Non-maintainer upload.
+ * Fix python bindings by making svn_stream_read() accept a long as its
+ second argument. Even if an int would be sufficient to store
+ SVN_STREAM_CHUNK_SIZE, this constant is stored as a long since it's
+ created through SWIG_From_long(), and users of svn_stream_read()
+ tend to pass it as a second parameter. The intent was to support
+ being passed a long anyway, but the prepended PyInt_Check() check
+ rejects non-int objects. Skip that test and let PyInt_AsLong() error
+ out. This should fix at least svnmailer, viewvc, trac. (Closes: #680298)
+ + patches/python-fix-svn_stream_read.diff
+
+ -- Cyril Brulebois <k...@debian.org> Tue, 17 Sep 2013 10:52:32 +0000
+
subversion (1.6.17dfsg-4+deb7u3) wheezy-security; urgency=high
* Non-maintainer upload by the Security Team.
diff -Nru subversion-1.6.17dfsg/debian/patches/python-fix-svn_stream_read.diff subversion-1.6.17dfsg/debian/patches/python-fix-svn_stream_read.diff
--- subversion-1.6.17dfsg/debian/patches/python-fix-svn_stream_read.diff 1970-01-01 00:00:00.000000000 +0000
+++ subversion-1.6.17dfsg/debian/patches/python-fix-svn_stream_read.diff 2013-09-17 10:58:34.000000000 +0000
@@ -0,0 +1,19 @@
+--- a/subversion/bindings/swig/core.i
++++ b/subversion/bindings/swig/core.i
+@@ -337,11 +337,11 @@
+ */
+ #ifdef SWIGPYTHON
+ %typemap(in) (char *buffer, apr_size_t *len) ($*2_type temp) {
+- if (!PyInt_Check($input)) {
+- PyErr_SetString(PyExc_TypeError,
+- "expecting an integer for the buffer size");
+- SWIG_fail;
+- }
++ /* skip calling PyInt_Check($input) here; it fails if the parameter is a
++ * long, which it tends to be, since it's created using SWIG_From_long;
++ * PyInt_AsLong already returns -1 if there's an error, and it's already
++ * taken care of in the following code (temp < 0).
++ */
+ temp = PyInt_AsLong($input);
+ if (temp < 0) {
+ PyErr_SetString(PyExc_ValueError,
diff -Nru subversion-1.6.17dfsg/debian/patches/series subversion-1.6.17dfsg/debian/patches/series
--- subversion-1.6.17dfsg/debian/patches/series 2013-09-17 10:58:33.000000000 +0000
+++ subversion-1.6.17dfsg/debian/patches/series 2013-09-17 10:58:34.000000000 +0000
@@ -40,3 +40,4 @@
cve-2013-1849
CVE-2013-1968.patch
CVE-2013-2112.patch
+python-fix-svn_stream_read.diff
