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
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/i6...
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
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/i6...
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@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@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@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@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.
LS
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/i6...
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@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@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@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@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
On Tue, Jan 12, 2016 at 03:21:30PM +0100, Lukas Slebodnik wrote:
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.
Garbage-collecting patches untouched for a week or more..
ACK
I tested with a local i386 build and put the patches to CI: http://sssd-ci.duckdns.org/logs/job/36/85/summary.html
The code also looks good to me, the pyhbac change is not public API, so it's OK to change the type.
On (28/01/16 21:54), Jakub Hrozek wrote:
On Tue, Jan 12, 2016 at 03:21:30PM +0100, Lukas Slebodnik wrote:
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.
Garbage-collecting patches untouched for a week or more..
ACK
I tested with a local i386 build and put the patches to CI: http://sssd-ci.duckdns.org/logs/job/36/85/summary.html
The code also looks good to me, the pyhbac change is not public API, so it's OK to change the type.
master: * f47a339d7794cd5a24d368b3b3640452686e45a5 * 2ff8131cf02decaf0dd0754e843732fe7774fc59 * d3f14ed93ef61268d0a68898ed9c44b4f773081c
LS
On (29/01/16 09:33), Lukas Slebodnik wrote:
On (28/01/16 21:54), Jakub Hrozek wrote:
On Tue, Jan 12, 2016 at 03:21:30PM +0100, Lukas Slebodnik wrote:
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.
Garbage-collecting patches untouched for a week or more..
ACK
I tested with a local i386 build and put the patches to CI: http://sssd-ci.duckdns.org/logs/job/36/85/summary.html
The code also looks good to me, the pyhbac change is not public API, so it's OK to change the type.
master:
- f47a339d7794cd5a24d368b3b3640452686e45a5
- 2ff8131cf02decaf0dd0754e843732fe7774fc59
- d3f14ed93ef61268d0a68898ed9c44b4f773081c
Backported to sssd-1-13 due to PR#51 * 085746864de710adfa506c342434722ad69c3d85 * 9341809d4428cd187aefdb2871afffb327447002 * 0257239c8504435961dc428f719d4d655ba4688d
LS
sssd-devel@lists.fedorahosted.org