New python PIDL checks cause 2221 new Coverity warnings
Stefan Metzmacher
metze at samba.org
Tue Sep 1 08:36:04 UTC 2015
Hi Andrew,
>> That looks like a start. Also,
>>
>> py_days_of_month = (uint32_t)object->days_of_month > LONG_MAX ?
>> PyLong_FromUnsignedLong((uint32_t)object->days_of_month) :
>> PyInt_FromLong((uint32_t)object->days_of_month);
>>
>> this pattern can easily be put into a helper function. In
>> addition to having just one coverity warning (or none if we
>> properly #ifdef this) we get less cluttered code to look at.
>
> Can you review and push the attached?
>
> This handles the unsigned case. The remaining case is for signed 64
> bit integers in IDL, which are vanishingly rare (6 instances), so I've
> left those in the generated output rather than add another helper
> function for those.
For symetric reasons I've also added a helper function for the signed
case.
I've also changed some leading whitespaces to tabs and added bug markers.
Everyone happy with the attached patches to be pushed to master?
> Once this is one, I would like to discuss getting the underlying
> changes into 4.3, as the purpose of this work is to allow AD sites
> using the sudo (and other custom) schema to upgrade their domains
> successfully.
Just attach the required patches to the bugreport and ask for review.
I'm also wondering if we want this also in 4.2.
metze
-------------- next part --------------
From 51773bd51c79c4270938220317d339746de4f430 Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Fri, 28 Aug 2015 11:46:56 +1200
Subject: [PATCH 1/5] pidl/python: Calculate maximum integer values using a
lookup table
This avoids a << of 64 bits in the unused end of the conditional expression.
This was flagged by Coverity and the fix was suggested by metze.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=11429
Andrew Bartlett
Signed-off-by: Andrew Bartlett <abartlet at samba.org>
Reviewed-by: Stefan Metzmacher <metze at samba.org>
---
pidl/lib/Parse/Pidl/Samba4/Python.pm | 41 ++++++++++++++++++++++++++++++++++--
1 file changed, 39 insertions(+), 2 deletions(-)
diff --git a/pidl/lib/Parse/Pidl/Samba4/Python.pm b/pidl/lib/Parse/Pidl/Samba4/Python.pm
index ad9ff88..2ace7a0 100644
--- a/pidl/lib/Parse/Pidl/Samba4/Python.pm
+++ b/pidl/lib/Parse/Pidl/Samba4/Python.pm
@@ -974,7 +974,7 @@ sub ConvertObjectFromPythonData($$$$$$;$)
|uid_t|gid_t)$/x) {
$self->pidl("{");
$self->indent;
- $self->pidl("const unsigned long long uint_max = (sizeof($target) == 8) ? UINT64_MAX : (unsigned long long)((1ULL << (sizeof($target) * 8)) - 1);");
+ $self->pidl("const unsigned long long uint_max = ndr_sizeof2uintmax(sizeof($target));");
$self->pidl("if (PyLong_Check($cvar)) {");
$self->indent;
$self->pidl("unsigned long long test_var;");
@@ -1025,7 +1025,7 @@ sub ConvertObjectFromPythonData($$$$$$;$)
if ($ctype_alias =~ /^(dlong|char|int[0-9]*|time_t)$/x) {
$self->pidl("{");
$self->indent;
- $self->pidl("const long long int_max = (long long)((1ULL << (sizeof($target) * 8 - 1)) - 1);");
+ $self->pidl("const long long int_max = ndr_sizeof2intmax(sizeof($target));");
$self->pidl("const long long int_min = -int_max - 1;");
$self->pidl("if (PyLong_Check($cvar)) {");
$self->indent;
@@ -1498,6 +1498,43 @@ sub Parse($$$$$)
#include \"$hdr\"
#include \"$ndr_hdr\"
+/*
+ * These functions are here to ensure they can be optomised out by
+ * the compiler based on the constant input values
+ */
+
+static inline unsigned long long ndr_sizeof2uintmax(size_t var_size)
+{
+ switch (var_size) {
+ case 8:
+ return UINT64_MAX;
+ case 4:
+ return UINT32_MAX;
+ case 2:
+ return UINT16_MAX;
+ case 1:
+ return UINT8_MAX;
+ }
+
+ return 0;
+}
+
+static inline long long ndr_sizeof2intmax(size_t var_size)
+{
+ switch (var_size) {
+ case 8:
+ return INT64_MAX;
+ case 4:
+ return INT32_MAX;
+ case 2:
+ return INT16_MAX;
+ case 1:
+ return INT8_MAX;
+ }
+
+ return 0;
+}
+
");
foreach my $x (@$ndr) {
--
1.9.1
From 281de494a7b00737b98f3d5653a70991c6815971 Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Tue, 1 Sep 2015 14:33:35 +1200
Subject: [PATCH 2/5] pidl/python: Provide static inline helper function
ndr_PyLong_FromUnsignedLongLong
This should isolate any coverity warnings on 64-bit platforms
(where LONG_MAX is larger than any possible 32 bit value) to
a single spot, or possibly eliminate it.
This is needed for the unsigned 64 bit case, and on 32 bit
systems, as PyInt_FromLong is limited to a signed "long" int.
The compiler should be able to eliminate many of these calls
with the embedded type knowlege.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=11429
Signed-off-by: Andrew Bartlett <abartlet at samba.org>
Reviewed-by: Stefan Metzmacher <metze at samba.org>
---
pidl/lib/Parse/Pidl/Samba4/Python.pm | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/pidl/lib/Parse/Pidl/Samba4/Python.pm b/pidl/lib/Parse/Pidl/Samba4/Python.pm
index 2ace7a0..056ab21 100644
--- a/pidl/lib/Parse/Pidl/Samba4/Python.pm
+++ b/pidl/lib/Parse/Pidl/Samba4/Python.pm
@@ -1284,7 +1284,7 @@ sub ConvertScalarToPython($$$)
}
if ($ctypename =~ /^(uint64|hyper|NTTIME_hyper|NTTIME|NTTIME_1sec|udlong|udlongr|uid_t|gid_t)$/) {
- return "$cvar > LONG_MAX ? PyLong_FromUnsignedLongLong($cvar) : PyInt_FromLong($cvar)";
+ return "ndr_PyLong_FromUnsignedLongLong($cvar)";
}
if ($ctypename =~ /^(char|int|int8|int16|int32|time_t)$/) {
@@ -1296,7 +1296,7 @@ sub ConvertScalarToPython($$$)
# possibly 64 bit unsigned long. (enums are signed in C,
# unsigned in NDR)
if ($ctypename =~ /^(uint32|uint3264)$/) {
- return "(uint32_t)$cvar > LONG_MAX ? PyLong_FromUnsignedLong((uint32_t)$cvar) : PyInt_FromLong((uint32_t)$cvar)";
+ return "ndr_PyLong_FromUnsignedLongLong((uint32_t)$cvar)";
}
if ($ctypename =~ /^(uint|uint8|uint16|uint1632)$/) {
@@ -1535,6 +1535,15 @@ static inline long long ndr_sizeof2intmax(size_t var_size)
return 0;
}
+static inline PyObject *ndr_PyLong_FromUnsignedLongLong(unsigned long long v)
+{
+ if (v > LONG_MAX) {
+ return PyLong_FromUnsignedLongLong(v);
+ } else {
+ return PyInt_FromLong(v);
+ }
+}
+
");
foreach my $x (@$ndr) {
@@ -1614,7 +1623,7 @@ static inline long long ndr_sizeof2intmax(size_t var_size)
my $py_obj;
my ($ctype, $cvar) = @{$h->{'val'}};
if ($cvar =~ /^[0-9]+$/ or $cvar =~ /^0x[0-9a-fA-F]+$/) {
- $py_obj = "$cvar > LONG_MAX ? PyLong_FromUnsignedLongLong($cvar) : PyInt_FromLong($cvar)";
+ $py_obj = "ndr_PyLong_FromUnsignedLongLong($cvar)";
} elsif ($cvar =~ /^".*"$/) {
$py_obj = "PyString_FromString($cvar)";
} else {
--
1.9.1
From adf48cf6fa7f5f07f00cc67fe34687af42c9c1c2 Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Tue, 1 Sep 2015 10:30:49 +0200
Subject: [PATCH 3/5] pidl/python: also add a ndr_PyLong_FromLongLong() for
symnetric reasons
BUG: https://bugzilla.samba.org/show_bug.cgi?id=11429
Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
pidl/lib/Parse/Pidl/Samba4/Python.pm | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/pidl/lib/Parse/Pidl/Samba4/Python.pm b/pidl/lib/Parse/Pidl/Samba4/Python.pm
index 056ab21..180b6b2 100644
--- a/pidl/lib/Parse/Pidl/Samba4/Python.pm
+++ b/pidl/lib/Parse/Pidl/Samba4/Python.pm
@@ -1280,7 +1280,7 @@ sub ConvertScalarToPython($$$)
$ctypename = expandAlias($ctypename);
if ($ctypename =~ /^(int64|dlong)$/) {
- return "($cvar > LONG_MAX || $cvar < LONG_MIN) ? PyLong_FromLongLong($cvar) : PyInt_FromLong($cvar)";
+ return "ndr_PyLong_FromLongLong($cvar)";
}
if ($ctypename =~ /^(uint64|hyper|NTTIME_hyper|NTTIME|NTTIME_1sec|udlong|udlongr|uid_t|gid_t)$/) {
@@ -1535,6 +1535,15 @@ static inline long long ndr_sizeof2intmax(size_t var_size)
return 0;
}
+static inline PyObject *ndr_PyLong_FromLongLong(long long v)
+{
+ if (v > LONG_MAX || v < LONG_MIN) {
+ return PyLong_FromLongLong(v);
+ } else {
+ return PyInt_FromLong(v);
+ }
+}
+
static inline PyObject *ndr_PyLong_FromUnsignedLongLong(unsigned long long v)
{
if (v > LONG_MAX) {
--
1.9.1
From 9915498358dcbd4f8e4babb325e968141d4c0462 Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Tue, 1 Sep 2015 14:58:20 +1200
Subject: [PATCH 4/5] python/tests: Add tests for 64 bit signed integers
BUG: https://bugzilla.samba.org/show_bug.cgi?id=11429
Signed-off-by: Andrew Bartlett <abartlet at samba.org>
Reviewed-by: Stefan Metzmacher <metze at samba.org>
---
python/samba/tests/dcerpc/integer.py | 28 +++++++++++++++++++++++++++-
1 file changed, 27 insertions(+), 1 deletion(-)
diff --git a/python/samba/tests/dcerpc/integer.py b/python/samba/tests/dcerpc/integer.py
index 1a392ee..f4fa482 100644
--- a/python/samba/tests/dcerpc/integer.py
+++ b/python/samba/tests/dcerpc/integer.py
@@ -17,7 +17,7 @@
"""Tests for integer handling in PIDL generated bindings samba.dcerpc.*"""
-from samba.dcerpc import server_id, misc, srvsvc
+from samba.dcerpc import server_id, misc, srvsvc, samr
import samba.tests
class IntegerTests(samba.tests.TestCase):
@@ -146,6 +146,32 @@ class IntegerTests(samba.tests.TestCase):
g.time_mid = misc.SV_TYPE_DOMAIN_ENUM
self.assertRaises(OverflowError, assign)
+ def test_hyper_into_int64(self):
+ s = samr.DomInfo1()
+ def assign():
+ s.max_password_age = server_id.SERVERID_UNIQUE_ID_NOT_TO_VERIFY
+ self.assertRaises(OverflowError, assign)
+
+ def test_int_into_int64(self):
+ s = samr.DomInfo1()
+ s.max_password_age = 5
+ self.assertEquals(s.max_password_age, 5)
+
+ def test_negative_int_into_int64(self):
+ s = samr.DomInfo1()
+ s.max_password_age = -5
+ self.assertEquals(s.max_password_age, -5)
+
+ def test_larger_int_into_int64(self):
+ s = samr.DomInfo1()
+ s.max_password_age = server_id.NONCLUSTER_VNN
+ self.assertEquals(s.max_password_age, 0xFFFFFFFFL)
+
+ def test_larger_negative_int_into_int64(self):
+ s = samr.DomInfo1()
+ s.max_password_age = -2147483649
+ self.assertEquals(s.max_password_age, -2147483649L)
+
def test_int_list_over_list(self):
g = misc.GUID()
g.node = [5, 0, 5, 0, 7, 4]
--
1.9.1
From bd3a22889cba21ac8b68a3df66c963b3cd3d5826 Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Tue, 1 Sep 2015 15:00:30 +1200
Subject: [PATCH 5/5] python/tests: Add more assertions that we get back the
value we expect
BUG: https://bugzilla.samba.org/show_bug.cgi?id=11429
Signed-off-by: Andrew Bartlett <abartlet at samba.org>
Reviewed-by: Stefan Metzmacher <metze at samba.org>
---
python/samba/tests/dcerpc/integer.py | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/python/samba/tests/dcerpc/integer.py b/python/samba/tests/dcerpc/integer.py
index f4fa482..ca5d571 100644
--- a/python/samba/tests/dcerpc/integer.py
+++ b/python/samba/tests/dcerpc/integer.py
@@ -30,6 +30,7 @@ class IntegerTests(samba.tests.TestCase):
def test_int_into_hyper(self):
s = server_id.server_id()
s.unique_id = 1
+ self.assertEquals(s.unique_id, 1)
def test_negative_int_into_hyper(self):
s = server_id.server_id()
@@ -52,6 +53,7 @@ class IntegerTests(samba.tests.TestCase):
def test_int_into_int32(self):
s = srvsvc.NetRemoteTODInfo()
s.timezone = 5
+ self.assertEquals(s.timezone, 5)
def test_uint32_into_int32(self):
s = srvsvc.NetRemoteTODInfo()
@@ -62,6 +64,7 @@ class IntegerTests(samba.tests.TestCase):
def test_long_into_int32(self):
s = srvsvc.NetRemoteTODInfo()
s.timezone = 5L
+ self.assertEquals(s.timezone, 5)
def test_larger_long_int_into_int32(self):
s = srvsvc.NetRemoteTODInfo()
@@ -72,6 +75,7 @@ class IntegerTests(samba.tests.TestCase):
def test_larger_int_into_int32(self):
s = srvsvc.NetRemoteTODInfo()
s.timezone = 2147483647
+ self.assertEquals(s.timezone, 2147483647)
def test_float_into_int32(self):
s = srvsvc.NetRemoteTODInfo()
@@ -88,6 +92,7 @@ class IntegerTests(samba.tests.TestCase):
def test_negative_int_into_int32(self):
s = srvsvc.NetRemoteTODInfo()
s.timezone = -2147483648
+ self.assertEquals(s.timezone, -2147483648)
def test_negative_into_uint32(self):
s = server_id.server_id()
@@ -128,6 +133,7 @@ class IntegerTests(samba.tests.TestCase):
def test_enum_into_uint16(self):
g = misc.GUID()
g.time_mid = misc.SEC_CHAN_DOMAIN
+ self.assertEquals(g.time_mid, misc.SEC_CHAN_DOMAIN)
def test_bitmap_into_uint16(self):
g = misc.GUID()
--
1.9.1
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20150901/46e538e0/signature.sig>
More information about the samba-technical
mailing list