Bug 1127310 - ERROR: testBinaryPayload (test.test_xattr.xattrTest)
ERROR: testBinaryPayload (test.test_xattr.xattrTest)
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: pyxattr (Show other bugs)
21
ppc64 Linux
unspecified Severity urgent
: ---
: ---
Assigned To: Marcin Zajaczkowski
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2014-08-06 11:38 EDT by Mark Hamzy
Modified: 2014-08-07 11:32 EDT (History)
10 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2014-08-07 11:32:01 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
proposed patch (4.12 KB, patch)
2014-08-06 15:16 EDT, Mark Hamzy
no flags Details | Diff

  None (edit)
Description Mark Hamzy 2014-08-06 11:38:52 EDT
python-2.7.7-3.fc21.ppc64p7
glibc-2.19.90-31.fc21.ppc64p7
attr-2.4.47-7.fc21.ppc64


Recently the testcases for pyxattr have been turned on.  Which results in the following:

http://ppc.koji.fedoraproject.org/kojifiles/work/tasks/7128/1977128/build.log
...
ERROR: testBinaryPayload (test.test_xattr.xattrTest)
test binary values
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/builddir/build/BUILD/pyxattr-0.5.3/test/test_xattr.py", line 351, in testBinaryPayload
    xattr.set(fname, self.USER_ATTR, BINVAL)
IOError: [Errno 22] Invalid argument
...


The function in question is:

...
static PyObject *
xattr_set(PyObject *self, PyObject *args, PyObject *keywds)
{
    PyObject *myarg, *res;
    int nofollow = 0;
    char *attrname = NULL;
    char *buf = NULL;
    int bufsize;
    int nret;
    int flags = 0;
    target_t tgt;
    const char *ns = NULL;
    char *newname;
    const char *full_name;
    static char *kwlist[] = {"item", "name", "value", "flags",
                             "nofollow", "namespace", NULL};

    /* Parse the arguments */
    if (!PyArg_ParseTupleAndKeywords(args, keywds, "Oetet#|ii" BYTES_CHAR,
                                     kwlist, &myarg, NULL, &attrname, NULL,
                                     &buf, &bufsize, &flags, &nofollow, &ns))
        return NULL;
    if(convert_obj(myarg, &tgt, nofollow) < 0) {
        res = NULL;
        goto freearg;
    }

    if(merge_ns(ns, attrname, &full_name, &newname) < 0) {
        res = NULL;
        goto freearg;
    }

    /* Set the attribute's value */
    nret = _set_obj(&tgt, full_name, buf, bufsize, flags);

    PyMem_Free(newname);

    free_tgt(&tgt);

    if(nret == -1) {
        res = PyErr_SetFromErrno(PyExc_IOError);
        goto freearg;
    }

    Py_INCREF(Py_None);
    res = Py_None;

 freearg:
    PyMem_Free(attrname);
    PyMem_Free(buf);

    /* Return the result */
    return res;
}
...


Running gdb on the testcase:


<mock-chroot>[root@bluebill ~]# (cd /builddir/build/BUILD/pyxattr-0.5.3/; export TEST_IGNORE_XATTRS=security.selinux; gdb /usr/bin/python2)
...
(gdb) path /builddir/build/BUILDROOT/pyxattr-0.5.3-1.fc21.ppc64/usr/lib64/python2.7/site-packages/
Executable and object file path: /builddir/build/BUILDROOT/pyxattr-0.5.3-1.fc21.ppc64/usr/lib64/python2.7/site-packages:/usr/local/sbin:/usr/bin:/bin:/usr/sbin:/sbin:/builddir/.local/bin:/builddir/bin
(gdb) break xattr_set
(gdb) run setup.py test
...
Breakpoint 1, xattr_set (self=0x0, args=('/builddir/build/BUILD/pyxattr-0.5.3/xattr-dhsdg0.test', u'user.test', 'abc\x00def'),
    keywds=0x0) at xattr.c:668
668     {
(gdb) print self
$1 = 0x0
(gdb) print args
$2 = ('/builddir/build/BUILD/pyxattr-0.5.3/xattr-dhsdg0.test', u'user.test', 'abc\x00def')
(gdb) print keywds
$3 = 0x0
(gdb) break _set_obj
Breakpoint 2 at 0x3fffaff32940: file xattr.c, line 216.
(gdb) c
Continuing.

Breakpoint 2, _set_obj (tgt=0x3fffffff8d90, name=0x10322cf0 "user.test", value=0x100f28b0, size=0, flags=7) at xattr.c:216
216                         const void *value, size_t size, int flags) {
(gdb) print value
$5 = (const void *) 0x100f28b0
(gdb) x/8b value
0x100f28b0:     0x61    0x62    0x63    0x00    0x64    0x65    0x66    0x00
(gdb) print size
$6 = 0
(gdb) print flags
$7 = 7



So it seems that bufsize should result in 7, but turns out to contain 0.
Comment 1 Dave Malcolm 2014-08-06 11:56:25 EDT
At a glance, I would guess that this is a type mismatch between format code "Oetet#|ii" in the call to PyArg_ParseTupleAndKeywords vs the underlying types of the variables being written to.

See https://docs.python.org/2/c-api/arg.html

In particular, "et#" is documented as taking:
 [const char *encoding, char **buffer, int *buffer_length]

which would seems to suggest that &bufsize ought to be an "int *".

However, "s#" has:

>Starting with Python 2.5 the type of the length argument can be controlled by >defining the macro PY_SSIZE_T_CLEAN before including Python.h. If the macro is >defined, length is a Py_ssize_t rather than an int.

and IIRC that *does* in fact affect "et#" and the other hash-suffixed codes i.e.  PyArg_ParseTupleAndKeywords was expecting bufsize to be a Py_ssize_t, not an int.

(see Python/getargs.c, and the "FETCH_SIZE" macro there; if PyArg_ParseTupleAndKeywords has been expanded to a call to PyArg_ParseTupleAndKeywords_SizeT that's what's going on).
Comment 2 Mark Hamzy 2014-08-06 14:18:01 EDT
I did see:

(gdb) s
_PyArg_ParseTupleAndKeywords_SizeT (args=('/builddir/build/BUILD/pyxattr-0.5.3/xattr-wKlyFZ.test', u'user.test', 'abc\x00def'),
    keywords=0x0, format=0x3fffaff33fd8 "Oetet#|iis", kwlist=0x3fffaff52ef8 <kwlist>)
    at /usr/src/debug/Python-2.7.7/Python/getargs.c:1454
1454        if ((args == NULL || !PyTuple_Check(args)) ||
Comment 3 Mark Hamzy 2014-08-06 14:21:51 EDT
And the top of the file contains:

#define PY_SSIZE_T_CLEAN
#include <Python.h>
Comment 4 Dave Malcolm 2014-08-06 14:30:44 EDT
(In reply to Mark Hamzy from comment #3)
> And the top of the file contains:
> 
> #define PY_SSIZE_T_CLEAN
> #include <Python.h>

Thanks.  The fix then, probably, is to change:
   int bufsize;
to:
   Py_ssize_t bufsize;
Comment 5 Mark Hamzy 2014-08-06 15:16:55 EDT
Created attachment 924571 [details]
proposed patch

[root@bluebill ~]# ppc-koji build --scratch f21 /home/mock/SHADOWBUILD-f21-build-repo_390420/root/builddir/build/SRPMS/pyxattr-0.5.3-1.fc21.src.rpm
Created task: 1978031
Task info: http://ppc.koji.fedoraproject.org/koji/taskinfo?taskID=1978031
...
1978031 build (f21, pyxattr-0.5.3-1.fc21.src.rpm) completed successfully

[root@bluebill ~]# koji build --scratch f21 /home/mock/SHADOWBUILD-f21-build-repo_390420/root/builddir/build/SRPMS/pyxattr-0.5.3-1.fc21.src.rpm
Created task: 7250277
Task info: http://koji.fedoraproject.org/koji/taskinfo?taskID=7250277
...
7250277 build (f21, pyxattr-0.5.3-1.fc21.src.rpm) completed successfully
Comment 6 Marcin Zajaczkowski 2014-08-06 18:07:48 EDT
Thanks guys for the proposed patch. I'm not a Python developer and I can't say how much it could impact builds for other architectures and the library itself. I sent a link to that issue to the pyxattr's author to ask for comments.
I would be nice to have it fixed upstream anyway.
Comment 7 Dan Horák 2014-08-07 03:21:37 EDT
Marcin, the other architectures just have enough luck they are not affected by the type mismatch :-) Please include the patch in the package ASAP, it blocks f21 (and rawhide later) builds on Fedora/ppc.
Comment 8 Marcin Zajaczkowski 2014-08-07 05:47:32 EDT
I applied the patch. Let me know if you need anything more with this package.
Comment 9 Dan Horák 2014-08-07 09:43:04 EDT
thanks, please do builds in both f21 and rawhide, the secondary arches will then inherit them automagically
Comment 10 Marcin Zajaczkowski 2014-08-07 10:50:38 EDT
I already did just after pushing the changes:
http://koji.fedoraproject.org/koji/buildinfo?buildID=551358
http://koji.fedoraproject.org/koji/buildinfo?buildID=551359

But maybe you are talking about some other kind of builds?
Comment 11 Dan Horák 2014-08-07 11:15:34 EDT
ah, thanks, then everything is done, I haven't checked koji because I saw the MODIFIED state which usually means changes committed, but not built yet. You can close the bug.
Comment 12 Marcin Zajaczkowski 2014-08-07 11:32:01 EDT
Ok.

Note You need to log in before you can comment on or make changes to this bug.