On (24/11/15 14:28), Lukas Slebodnik wrote:
On (23/11/15 15:29), Lukas Slebodnik wrote:
>On (20/11/15 10:17), Lukas Slebodnik wrote:
>>ehlo,
>>
>>I found few warnings on rawhide when I build sssd-1.13.2
>>https://kojipkgs.fedoraproject.org//work/tasks/8618/11918618/build.log
>>
>>LS
>
>I missed one warning in pysss_nss_idmap.cwhich was not visible
>on 64 bit architecture.
>
>@see 32 bit build log
>https://kojipkgs.fedoraproject.org//packages/sssd/1.13.2/1.fc24/data/logs/i686/build.log
>
>BTW warning are caused by newly added warnings in python3.5 cflags
>
>PYTHON3_CFLAGS='-I/usr/include/python3.5m -I/usr/include/python3.5m
>-Wno-unused-result -Wsign-compare -Wunreachable-code -O2 -g -pipe -Wall
>-Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions
>-fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches
>-specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m32 -march=i686 -mtune=atom
>-fasynchronous-unwind-tables -D_GNU_SOURCE -fPIC -fwrapv
>-DDYNAMIC_ANNOTATIONS_ENABLED=1 -DNDEBUG -O2 -g -pipe -Wall
>-Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions
>-fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches
>-specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m32 -march=i686 -mtune=atom
>-fasynchronous-unwind-tables -D_GNU_SOURCE -fPIC -fwrapv'
>
>
>python3.5 is only on rawhide.
>
>Updated patches are attached.
>
>LS
>From c3e22efaacac525476ade16fd7d81bdfe7115e5c Mon Sep 17 00:00:00 2001
>From: Lukas Slebodnik <lslebodn(a)redhat.com>
>Date: Thu, 19 Nov 2015 13:07:10 +0000
>Subject: [PATCH 1/3] TOOLS: Fix warning Wsign-compare
>MIME-Version: 1.0
>Content-Type: text/plain; charset=UTF-8
>Content-Transfer-Encoding: 8bit
>
>src/tools/tools_util.c: In function ‘parse_groups’:
>src/tools/tools_util.c:116:19: error: comparison between
> signed and unsigned integer expressions [-Werror=sign-compare]
> for (i = 0; i < tokens; i++) {
> ^
>---
> src/tools/tools_util.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/src/tools/tools_util.c b/src/tools/tools_util.c
>index
f9dca728751f0429bb2f1c3ef46ffc269ecfba40..f475a51ef380f5d0397deb22d161fd2c8d2a81b8 100644
>--- a/src/tools/tools_util.c
>+++ b/src/tools/tools_util.c
>@@ -94,7 +94,7 @@ int parse_groups(TALLOC_CTX *mem_ctx, const char *optstr, char
***_out)
> char *orig, *n, *o;
> char delim = ',';
> unsigned int tokens = 1;
>- int i;
>+ unsigned int i;
>
> orig = talloc_strdup(mem_ctx, optstr);
> if (!orig) return ENOMEM;
>--
>2.5.0
>
>From b79f1fe6f6ffec7b9a7c9f045c12bc6c894faa78 Mon Sep 17 00:00:00 2001
>From: Lukas Slebodnik <lslebodn(a)redhat.com>
>Date: Thu, 19 Nov 2015 13:07:52 +0000
>Subject: [PATCH 2/3] pysss_murmur: Fix warning Wsign-compare
>MIME-Version: 1.0
>Content-Type: text/plain; charset=UTF-8
>Content-Transfer-Encoding: 8bit
>
>src/python/pysss_murmur.c: In function ‘py_murmurhash3’:
>src/python/pysss_murmur.c:47:17: error: comparison between
> signed and unsigned integer expressions [-Werror=sign-compare]
> key_len > strlen(key)) {
> ^
>
>uint32_t murmurhash3(const char *key, int len, uint32_t seed)
>The second argument of the function murmurhash3 has type int.
>But the code expects to be unsigned integer.
>
>There is code in python wrapper py_murmurhash3
>which check boundaries of that argument.
>It should be an unsigned "key_len > INT_MAX || key_len < 0".
>An exception should be thrown for negative number.
>
>Moreover, the length should be shorter then a length of input string.
>The strlen returns size_t which is unsigned and key_len is signed long.
>We already checked that value is unsigned so
>we can safely cast key_len to size_t
>---
> src/python/pysss_murmur.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/src/python/pysss_murmur.c b/src/python/pysss_murmur.c
>index
97d752b2a7734a332a5d5da07d75b594638015c8..b14a672025c218ae3ab314c3ad2cf2c5ced40870 100644
>--- a/src/python/pysss_murmur.c
>+++ b/src/python/pysss_murmur.c
>@@ -44,7 +44,7 @@ static PyObject * py_murmurhash3(PyObject *module, PyObject *args)
> }
>
> if (seed > UINT32_MAX || key_len > INT_MAX || key_len < 0 ||
>- key_len > strlen(key)) {
>+ (size_t)key_len > strlen(key)) {
> PyErr_Format(PyExc_ValueError, "Invalid value\n");
> return NULL;
> }
>--
>2.5.0
>
>From 07d5ea89b83e0176f7e51bd8b0ae9881aadff3bf Mon Sep 17 00:00:00 2001
>From: Lukas Slebodnik <lslebodn(a)redhat.com>
>Date: Thu, 19 Nov 2015 14:17:36 +0000
>Subject: [PATCH 3/3] pyhbac: Fix warning Wsign-compare
>MIME-Version: 1.0
>Content-Type: text/plain; charset=UTF-8
>Content-Transfer-Encoding: 8bit
>
>src/python/pyhbac.c: In function ‘HbacRuleElement_repr’:
>src/python/pyhbac.c:506:59: error: comparison between
> signed and unsigned integer expressions [-Werror=sign-compare]
> if (strnames == NULL || strgroups == NULL || category == -1) {
> ^
>src/python/pyhbac.c: In function ‘HbacRuleElement_to_native’:
>src/python/pyhbac.c:614:51: error: comparison between
> signed and unsigned integer expressions [-Werror=sign-compare]
> if (!el->names || !el->groups || el->category == -1) {
> ^
>
>The static function native_category had type of terurn value uint32_t
>But it also could return -1 which indicated an error.
>
>It's better to don't mix return code with returned value.
>---
> src/python/pyhbac.c | 18 +++++++++++-------
> 1 file changed, 11 insertions(+), 7 deletions(-)
>
>diff --git a/src/python/pyhbac.c b/src/python/pyhbac.c
>index
fca476c56ba697693c793ecad7945ae7edf3dffd..820ef11b57f1226725fd7acf97598a42e6bf0bc0 100644
>--- a/src/python/pyhbac.c
>+++ b/src/python/pyhbac.c
>@@ -191,8 +191,8 @@ pyobject_to_category(PyObject *o)
> return -1;
> }
>
>-static uint32_t
>-native_category(PyObject *pycat)
>+static int
>+native_category(PyObject *pycat, uint32_t *_category)
> {
> PyObject *iterator;
> PyObject *item;
>@@ -218,7 +218,9 @@ native_category(PyObject *pycat)
> }
>
> Py_DECREF(iterator);
>- return cat;
>+
>+ *_category = cat;
>+ return 0;
> }
>
> static char *
>@@ -491,6 +493,7 @@ HbacRuleElement_repr(HbacRuleElement *self)
> char *strnames = NULL;
> char *strgroups = NULL;
> uint32_t category;
>+ int ret;
> PyObject *o, *format, *args;
>
> format = PyUnicode_FromString("<category %lu names [%s] groups
[%s]>");
>@@ -502,8 +505,8 @@ HbacRuleElement_repr(HbacRuleElement *self)
> discard_const_p(char, ","));
> strgroups = str_concat_sequence(self->groups,
> discard_const_p(char, ","));
>- category = native_category(self->category);
>- if (strnames == NULL || strgroups == NULL || category == -1) {
>+ ret = native_category(self->category, &category);
>+ if (strnames == NULL || strgroups == NULL || ret == -1) {
> PyMem_Free(strnames);
> PyMem_Free(strgroups);
> Py_DECREF(format);
>@@ -592,6 +595,7 @@ struct hbac_rule_element *
> HbacRuleElement_to_native(HbacRuleElement *pyel)
> {
> struct hbac_rule_element *el = NULL;
>+ int ret;
>
> /* check the type, None would wreak havoc here because for some reason
> * it would pass the sequence check */
>@@ -608,10 +612,10 @@ HbacRuleElement_to_native(HbacRuleElement *pyel)
> goto fail;
> }
>
>- el->category = native_category(pyel->category);
>+ ret = native_category(pyel->category, &el->category);
> el->names = sequence_as_string_list(pyel->names, "names");
> el->groups = sequence_as_string_list(pyel->groups, "groups");
>- if (!el->names || !el->groups || el->category == -1) {
>+ if (!el->names || !el->groups || ret == -1) {
> goto fail;
> }
>
>--
>2.5.0
>
>From 8612123c0c57db3dcb4e7138e299ab0f8f5e4ebb Mon Sep 17 00:00:00 2001
>From: Lukas Slebodnik <lslebodn(a)redhat.com>
>Date: Mon, 23 Nov 2015 14:17:05 +0000
>Subject: [PATCH 4/4] pysss_nss_idmap: Fix warning Wsign-compare
>
>We get id from python with type long.
>We need to checked if we can safely cast to unsigned 32 bit integer.
>
>src/python/pysss_nss_idmap.c: In function 'do_getsidbyid':
>src/python/pysss_nss_idmap.c:155:22: warning:
> comparison between signed and unsigned integer expressions [-Wsign-compare]
> if (id < 0 || id > UINT32_MAX) {
> ^
NACK to 4th patch.
I would say it's but in do_getsidbyid.
The size of "long" is 4 bytes on i686.
It's the same size as size of "int"
So we filter out ID higher than 1073741824 on i686.
python-3.5 is also in debian-sid and migh be in debian-testing
within 10 days. First 3 patches can be reviwed and meanwhile
I will need to figure out solution for 4th patch (but it's only problem for
32 bit platforms)
The first 3 patches are attached.
LS