iustin / pylibacl

A python module for manipulating POSIX.1e ACLs
http://pylibacl.k1024.org/
GNU Lesser General Public License v2.1
21 stars 10 forks source link

AssertionError: OverflowError not raised in tests on i686 and armv7hl #13

Closed szpak closed 4 years ago

szpak commented 4 years ago

Upgrading the pylibacl package in Fedora to 0.5.4 I've got:

============================= test session starts ==============================
platform linux -- Python 3.8.0, pytest-4.6.6, py-1.8.0, pluggy-0.12.0
rootdir: /builddir/build/BUILD/pylibacl-0.5.4
collected 51 items
test/test_acls.py ............s..............s.........F.............    [100%]
=================================== FAILURES ===================================
___________________ ModificationTests.testNegativeQualifier ____________________
self = <test.test_acls.ModificationTests testMethod=testNegativeQualifier>
    @unittest.skipUnless(IS_PY_3K, "Only supported under Python 3")
    def testNegativeQualifier(self):
        """Tests negative qualifier handling"""
        # Note: this presumes that uid_t/gid_t in C are unsigned...
        acl = posix1e.ACL()
        e = acl.append()
        for tag in [posix1e.ACL_USER, posix1e.ACL_GROUP]:
            e.tag_type = tag
            for qualifier in [-10, -5, -1]:
                with self.assertRaises(OverflowError):
>                   e.qualifier = qualifier
E                   AssertionError: OverflowError not raised
test/test_acls.py:616: AssertionError
================ 1 failed, 48 passed, 2 skipped in 0.16 seconds ================

for the build on i686 and armv7hl. Other builds work fine. The build for 0.5.2 was fine 3 months ago, but there could be some dependency upgraded in the meantime, so I may re-request the 0.5.2 build with current rawhide (if needed to verify anything).

I wonder if it is only some problem with tests or something more serious.

iustin commented 4 years ago

If you moved from 0.5.2 to 0.5.4, then this test is new, it was added in 0.5.3.

And I just realise I'm not testing at all with 32 bit anymore; all my builds are done with 64 bit. The overflow test is a bit hack-ish, I guess my assumption of overflow are not sound…

Yes, I've checked and the overflow check in the C code is basically broken, but passing on 64 bit. I'd suggest just commenting out this test for now.

By the way, I'm preparing a new upstream release dropping Python 2 compatibility; would that cause a problem for Fedora?

szpak commented 4 years ago

Thanks for your suggestion. I skipped that test and the build passed.

By the way, I'm preparing a new upstream release dropping Python 2 compatibility; would that cause a problem for Fedora?

Not at all. Actually, Fedora is removing the Python 2 packages in for the next Fedora version. Therefor, for Fedora itself it will perfectly fine :).

hroncok commented 4 years ago

The overflow check, as implemented:

#include <stdio.h>
#include <sys/types.h>

int main() {
    gid_t gid;
    long l;
    l = -5;
    gid = l;
    if((long)gid != l) printf("!=\n");
    else printf("==\n");
    return 0;
}
$ gcc -m32 test.c -o 32
$ gcc -m64 test.c -o 64

$ ./64
!=
$ ./32
==
iustin commented 4 years ago

Yes, that's what I found in my testing as well. It works well for positive overflow (long is bigger than uid_t which is int) but not for negative. I will fix this by switching negative test to Python built-in, and keep the current one for positive.

iustin commented 4 years ago

While trying to fix this, I found out that—on systems where uid_t/gid_t are unsigned, such a Linux—not only setting, but also retrieving the qualifier has bugs related to signedness and values outside the range of the signed values. Fun!

iustin commented 4 years ago

This is fixed in git head, the fix will be released in 0.6 - I expect a couple of weeks.