Discussion:
[PATCH] Enforce strict overflow checking
Andreas Schneider via samba-technical
2018-03-02 09:02:22 UTC
Permalink
Hello,

in a lot of places we use the incorrect size types, which could lead to
issues. This is mostly int vs. size_t.

-Werror=strict-overflow -Wstrict-overflow=2

this allows us to use enforce the correct size types. We could discuss moving
to -Wstrict-overflow=3 but higher values don't make sense. You also get a
false positives with 4 or 5.


The ms_fnmatch() patchset needs to be in master first, as it addresses a
strict-overflow issue (currently in autobuild).


Review is much appreciated.


Thanks,


Andreas
--
Andreas Schneider GPG-ID: CC014E3D
Samba Team ***@samba.org
www.samba.org
Andrew Bartlett via samba-technical
2018-03-02 09:16:22 UTC
Permalink
On Fri, 2018-03-02 at 10:02 +0100, Andreas Schneider via samba-
Post by Andreas Schneider via samba-technical
Hello,
in a lot of places we use the incorrect size types, which could lead to
issues. This is mostly int vs. size_t.
-Werror=strict-overflow -Wstrict-overflow=2
this allows us to use enforce the correct size types. We could discuss moving
to -Wstrict-overflow=3 but higher values don't make sense. You also get a
false positives with 4 or 5.
The ms_fnmatch() patchset needs to be in master first, as it addresses a
strict-overflow issue (currently in autobuild).
Review is much appreciated.
Thanks for the Heimdal pull request. I've got merge rights there so
bug me if this doesn't land early next week.
Post by Andreas Schneider via samba-technical
From 9042bdc220419ba8cdef37fa942562261fadca5a Mon Sep 17 00:00:00 2001
Date: Thu, 7 Dec 2017 16:40:51 +0100
Subject: [PATCH 32/39] lib:param: Fix P_LIST case in set_variable_helper()
This fixes compilation with -Wstrict-overflow=2
and
Post by Andreas Schneider via samba-technical
Subject: [PATCH 35/39] s3:locking: Fix integer overflow check in
posix_lock_in_range()
This fixes compilation with -Wstrict-overflow=2
---
source3/locking/posix.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
and
Post by Andreas Schneider via samba-technical
From f3dea9f7382ddee9307146d3beeb97c9b3f2ba78 Mon Sep 17 00:00:00 2001
Date: Thu, 7 Dec 2017 18:01:45 +0100
Subject: [PATCH 36/39] s3:printing: Fix size check in get_file_version()
This fixes compilation with -Wstrict-overflow=2
which might be fine, but it is after 10pm here and I'm not actually a
night-owl :-)

Reviewed-by: Andrew Bartlett <***@samba.org>

As this is about integer overflow, another set of eyes would be good,
but not compulsory.

Thanks,

Andrew Bartlett
--
Andrew Bartlett http://samba.org/~abartlet/
Authentication Developer, Samba Team http://samba.org
Samba Developer, Catalyst IT http://catalyst.net.nz/services/samba
Andreas Schneider via samba-technical
2018-03-02 15:25:24 UTC
Permalink
On Friday, 2 March 2018 10:16:22 CET Andrew Bartlett via samba-technical
Post by Andrew Bartlett via samba-technical
On Fri, 2018-03-02 at 10:02 +0100, Andreas Schneider via samba-
Post by Andreas Schneider via samba-technical
Hello,
in a lot of places we use the incorrect size types, which could lead to
issues. This is mostly int vs. size_t.
-Werror=strict-overflow -Wstrict-overflow=2
this allows us to use enforce the correct size types. We could discuss
moving to -Wstrict-overflow=3 but higher values don't make sense. You
also get a false positives with 4 or 5.
The ms_fnmatch() patchset needs to be in master first, as it addresses a
strict-overflow issue (currently in autobuild).
Review is much appreciated.
Thanks for the Heimdal pull request. I've got merge rights there so
bug me if this doesn't land early next week.
Post by Andreas Schneider via samba-technical
From 9042bdc220419ba8cdef37fa942562261fadca5a Mon Sep 17 00:00:00 2001
Date: Thu, 7 Dec 2017 16:40:51 +0100
Subject: [PATCH 32/39] lib:param: Fix P_LIST case in set_variable_helper()
This fixes compilation with -Wstrict-overflow=2
and
Post by Andreas Schneider via samba-technical
Subject: [PATCH 35/39] s3:locking: Fix integer overflow check in
posix_lock_in_range()
This fixes compilation with -Wstrict-overflow=2
---
source3/locking/posix.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
and
Post by Andreas Schneider via samba-technical
From f3dea9f7382ddee9307146d3beeb97c9b3f2ba78 Mon Sep 17 00:00:00 2001
Date: Thu, 7 Dec 2017 18:01:45 +0100
Subject: [PATCH 36/39] s3:printing: Fix size check in get_file_version()
This fixes compilation with -Wstrict-overflow=2
which might be fine, but it is after 10pm here and I'm not actually a
night-owl :-)
As this is about integer overflow, another set of eyes would be good,
but not compulsory.
Thanks for the quick review, and yes, more eyes are much appreciated
especially for changes mentioned above.



Andreas
--
Andreas Schneider GPG-ID: CC014E3D
Samba Team ***@samba.org
www.samba.org
Andreas Schneider via samba-technical
2018-03-09 07:51:46 UTC
Permalink
On Friday, 2 March 2018 16:25:24 CET Andreas Schneider via samba-technical
Post by Andreas Schneider via samba-technical
On Friday, 2 March 2018 10:16:22 CET Andrew Bartlett via samba-technical
Post by Andrew Bartlett via samba-technical
On Fri, 2018-03-02 at 10:02 +0100, Andreas Schneider via samba-
Post by Andreas Schneider via samba-technical
Hello,
in a lot of places we use the incorrect size types, which could lead to
issues. This is mostly int vs. size_t.
-Werror=strict-overflow -Wstrict-overflow=2
this allows us to use enforce the correct size types. We could discuss
moving to -Wstrict-overflow=3 but higher values don't make sense. You
also get a false positives with 4 or 5.
The ms_fnmatch() patchset needs to be in master first, as it addresses a
strict-overflow issue (currently in autobuild).
Review is much appreciated.
Thanks for the Heimdal pull request. I've got merge rights there so
bug me if this doesn't land early next week.
Post by Andreas Schneider via samba-technical
From 9042bdc220419ba8cdef37fa942562261fadca5a Mon Sep 17 00:00:00 2001
Date: Thu, 7 Dec 2017 16:40:51 +0100
Subject: [PATCH 32/39] lib:param: Fix P_LIST case in
set_variable_helper()
This fixes compilation with -Wstrict-overflow=2
and
Post by Andreas Schneider via samba-technical
Subject: [PATCH 35/39] s3:locking: Fix integer overflow check in
posix_lock_in_range()
This fixes compilation with -Wstrict-overflow=2
---
source3/locking/posix.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
and
Post by Andreas Schneider via samba-technical
From f3dea9f7382ddee9307146d3beeb97c9b3f2ba78 Mon Sep 17 00:00:00 2001
Date: Thu, 7 Dec 2017 18:01:45 +0100
Subject: [PATCH 36/39] s3:printing: Fix size check in get_file_version()
This fixes compilation with -Wstrict-overflow=2
which might be fine, but it is after 10pm here and I'm not actually a
night-owl :-)
As this is about integer overflow, another set of eyes would be good,
but not compulsory.
Thanks for the quick review, and yes, more eyes are much appreciated
especially for changes mentioned above.
Could someone please review?


Thanks!
--
Andreas Schneider GPG-ID: CC014E3D
Samba Team ***@samba.org
www.samba.org
Jeremy Allison via samba-technical
2018-03-13 00:15:49 UTC
Permalink
Post by Andreas Schneider via samba-technical
On Friday, 2 March 2018 16:25:24 CET Andreas Schneider via samba-technical
Post by Andreas Schneider via samba-technical
On Friday, 2 March 2018 10:16:22 CET Andrew Bartlett via samba-technical
Post by Andrew Bartlett via samba-technical
On Fri, 2018-03-02 at 10:02 +0100, Andreas Schneider via samba-
Post by Andreas Schneider via samba-technical
Hello,
in a lot of places we use the incorrect size types, which could lead to
issues. This is mostly int vs. size_t.
-Werror=strict-overflow -Wstrict-overflow=2
this allows us to use enforce the correct size types. We could discuss
moving to -Wstrict-overflow=3 but higher values don't make sense. You
also get a false positives with 4 or 5.
The ms_fnmatch() patchset needs to be in master first, as it addresses a
strict-overflow issue (currently in autobuild).
Review is much appreciated.
Thanks for the Heimdal pull request. I've got merge rights there so
bug me if this doesn't land early next week.
Post by Andreas Schneider via samba-technical
From 9042bdc220419ba8cdef37fa942562261fadca5a Mon Sep 17 00:00:00 2001
Date: Thu, 7 Dec 2017 16:40:51 +0100
Subject: [PATCH 32/39] lib:param: Fix P_LIST case in
set_variable_helper()
This fixes compilation with -Wstrict-overflow=2
and
Post by Andreas Schneider via samba-technical
Subject: [PATCH 35/39] s3:locking: Fix integer overflow check in
posix_lock_in_range()
This fixes compilation with -Wstrict-overflow=2
---
source3/locking/posix.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
and
Post by Andreas Schneider via samba-technical
From f3dea9f7382ddee9307146d3beeb97c9b3f2ba78 Mon Sep 17 00:00:00 2001
Date: Thu, 7 Dec 2017 18:01:45 +0100
Subject: [PATCH 36/39] s3:printing: Fix size check in get_file_version()
This fixes compilation with -Wstrict-overflow=2
which might be fine, but it is after 10pm here and I'm not actually a
night-owl :-)
As this is about integer overflow, another set of eyes would be good,
but not compulsory.
Thanks for the quick review, and yes, more eyes are much appreciated
especially for changes mentioned above.
Could someone please review?
I will try and get to this this week. Ping me if you
don't see a response.

Sorry for the delay,

Jeremy.
Andreas Schneider via samba-technical
2018-03-15 09:16:11 UTC
Permalink
On Fri, Mar 09, 2018 at 08:51:46AM +0100, Andreas Schneider via samba-
Post by Andreas Schneider via samba-technical
On Friday, 2 March 2018 16:25:24 CET Andreas Schneider via samba-technical
Post by Andreas Schneider via samba-technical
On Friday, 2 March 2018 10:16:22 CET Andrew Bartlett via samba-technical
Post by Andrew Bartlett via samba-technical
On Fri, 2018-03-02 at 10:02 +0100, Andreas Schneider via samba-
Post by Andreas Schneider via samba-technical
Hello,
in a lot of places we use the incorrect size types, which could lead to
issues. This is mostly int vs. size_t.
-Werror=strict-overflow -Wstrict-overflow=2
this allows us to use enforce the correct size types. We could discuss
moving to -Wstrict-overflow=3 but higher values don't make sense. You
also get a false positives with 4 or 5.
The ms_fnmatch() patchset needs to be in master first, as it addresses a
strict-overflow issue (currently in autobuild).
Review is much appreciated.
Thanks for the Heimdal pull request. I've got merge rights there so
bug me if this doesn't land early next week.
Post by Andreas Schneider via samba-technical
From 9042bdc220419ba8cdef37fa942562261fadca5a Mon Sep 17 00:00:00 2001
Date: Thu, 7 Dec 2017 16:40:51 +0100
Subject: [PATCH 32/39] lib:param: Fix P_LIST case in
set_variable_helper()
This fixes compilation with -Wstrict-overflow=2
and
Post by Andreas Schneider via samba-technical
Subject: [PATCH 35/39] s3:locking: Fix integer overflow check in
posix_lock_in_range()
This fixes compilation with -Wstrict-overflow=2
---
source3/locking/posix.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
and
Post by Andreas Schneider via samba-technical
From f3dea9f7382ddee9307146d3beeb97c9b3f2ba78 Mon Sep 17 00:00:00 2001
Date: Thu, 7 Dec 2017 18:01:45 +0100
Subject: [PATCH 36/39] s3:printing: Fix size check in
get_file_version()
This fixes compilation with -Wstrict-overflow=2
which might be fine, but it is after 10pm here and I'm not actually a
night-owl :-)
As this is about integer overflow, another set of eyes would be good,
but not compulsory.
Thanks for the quick review, and yes, more eyes are much appreciated
especially for changes mentioned above.
Could someone please review?
I will try and get to this this week. Ping me if you
don't see a response.
This is a ping :-)


Thanks for the review!
Jeremy Allison via samba-technical
2018-03-20 20:18:39 UTC
Permalink
Post by Andreas Schneider via samba-technical
This is a ping :-)
Thanks for the review!
OK, the only one I'm pushing back on is this:

--------------------------------------------------------------------------------
From: Andreas Schneider <***@samba.org>
Date: Thu, 7 Dec 2017 18:01:45 +0100
Subject: [PATCH 36/39] s3:printing: Fix size check in get_file_version()

This fixes compilation with -Wstrict-overflow=2

Signed-off-by: Andreas Schneider <***@samba.org>
---
source3/printing/nt_printing.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/source3/printing/nt_printing.c b/source3/printing/nt_printing.c
index 2e500f18c7d..00c737d2fd4 100644
--- a/source3/printing/nt_printing.c
+++ b/source3/printing/nt_printing.c
@@ -485,7 +485,7 @@ static int get_file_version(files_struct *fsp, char *fname,uint32_t *major, uint
/* Potential match data crosses buf boundry, move it to beginning
* of buf, and fill the buf with as much as it will hold. */
if (i>byte_count-VS_VERSION_INFO_SIZE) {
- int bc;
+ ssize_t bc;

memcpy(buf, &buf[i], byte_count-i);
if ((bc = vfs_read_data(fsp, &buf[byte_count-i], VS_NE_BUF_SIZE-
@@ -496,8 +496,10 @@ static int get_file_version(files_struct *fsp, char *fname,uint32_t *major, uint
goto error_exit;
}

+ if (byte_count - i < VS_VERSION_INFO_SIZE) {
+ break;
+ }
byte_count = bc + (byte_count - i);
- if (byte_count<VS_VERSION_INFO_SIZE) break;

i = 0;
}

--------------------------------------------------------------------------------

The removal of:

- if (byte_count<VS_VERSION_INFO_SIZE) break;

and addition of:

+ if (byte_count - i < VS_VERSION_INFO_SIZE) {
+ break;
+ }

check different conditions (i.e. leads to a change
in behavior). The original check is to ensure we
now have at least VS_VERSION_INFO_SIZE bytes in
buf[], including the value we just read of length
VS_NE_BUF_SIZE-(byte_count-i). Your new check
only checks that the remaining buffer size
we just filled is less than VS_VERSION_INFO_SIZE,
which isn't the same thing as checking that we
now have at least VS_VERSION_INFO_SIZE bytes in
the buffer.

Can you review the following fix instead ? It
fixes the memcpy -> memmove bug, plus it renames
everything to helper variables that should make
this code clearer to understand.

Thanks !

Jeremy.
Jeremy Allison via samba-technical
2018-03-20 21:38:44 UTC
Permalink
Post by Andreas Schneider via samba-technical
This is a ping :-)
Thanks for the review!
Having said that - [PATCH 32/39] lib:param: Fix P_LIST case in set_variable_helper()
causes:

make test TESTS=samba.tests.docs

to fail almost immediately, so that one needs looking at (sorry, it
must change the logic somewhere but I can't immediately see how).

Jeremy.
Andreas Schneider via samba-technical
2018-03-21 11:21:16 UTC
Permalink
Post by Jeremy Allison via samba-technical
Post by Andreas Schneider via samba-technical
This is a ping :-)
Thanks for the review!
Having said that - [PATCH 32/39] lib:param: Fix P_LIST case in
Here are the latest fixes addressing the remaining issues.

:-)


Please check. Thanks!
--
Andreas Schneider GPG-ID: CC014E3D
Samba Team ***@samba.org
www.samba.org
Jeremy Allison via samba-technical
2018-03-21 17:12:37 UTC
Permalink
Post by Andreas Schneider via samba-technical
On Tue, Mar 20, 2018 at 01:18:39PM -0700, Jeremy Allison via samba-technical
Post by Andreas Schneider via samba-technical
This is a ping :-)
Thanks for the review!
Having said that - [PATCH 32/39] lib:param: Fix P_LIST case in
Here are the latest fixes addressing the remaining issues.
:-)
Please check. Thanks!
Reviewed-by: Jeremy Allison <***@samba.org>

and pushed. Thanks !

Jeremy.
Post by Andreas Schneider via samba-technical
--
Andreas Schneider GPG-ID: CC014E3D
www.samba.org
From a1df5b7472c017e855e161b843c12c56efbfb947 Mon Sep 17 00:00:00 2001
Date: Thu, 7 Dec 2017 18:01:45 +0100
Subject: [PATCH 1/6] s3:printing: Fix size check in get_file_version()
This fixes compilation with -Wstrict-overflow=2
---
source3/printing/nt_printing.c | 24 ++++++++++++++++++------
1 file changed, 18 insertions(+), 6 deletions(-)
diff --git a/source3/printing/nt_printing.c b/source3/printing/nt_printing.c
index 2e500f18c7d..241af37743e 100644
--- a/source3/printing/nt_printing.c
+++ b/source3/printing/nt_printing.c
@@ -485,19 +485,31 @@ static int get_file_version(files_struct *fsp, char *fname,uint32_t *major, uint
/* Potential match data crosses buf boundry, move it to beginning
* of buf, and fill the buf with as much as it will hold. */
if (i>byte_count-VS_VERSION_INFO_SIZE) {
- int bc;
+ ssize_t amount_read;
+ ssize_t amount_unused = byte_count-i;
- memcpy(buf, &buf[i], byte_count-i);
- if ((bc = vfs_read_data(fsp, &buf[byte_count-i], VS_NE_BUF_SIZE-
- (byte_count-i))) < 0) {
+ memmove(buf, &buf[i], amount_unused);
+ amount_read = vfs_read_data(fsp,
+ &buf[amount_unused],
+ VS_NE_BUF_SIZE- amount_unused);
+ if (amount_read < 0) {
DEBUG(0,("get_file_version: NE file [%s] Read error, errno=%d\n",
fname, errno));
goto error_exit;
}
- byte_count = bc + (byte_count - i);
- if (byte_count<VS_VERSION_INFO_SIZE) break;
+ if (amount_read + amount_unused <
+ amount_read) {
+ /* Check for integer wrap. */
+ break;
+ }
+
+ byte_count = amount_read +
+ amount_unused;
+ if (byte_count < VS_VERSION_INFO_SIZE) {
+ break;
+ }
i = 0;
}
--
2.16.2
From 921fad52898deeb0edb4c275e0ff4aac3a0a792d Mon Sep 17 00:00:00 2001
Date: Wed, 21 Mar 2018 11:19:44 +0100
Subject: [PATCH 2/6] s3:lib: Fix size types in ms_fnmatch()
This fixes compilation with -Wstrict-overflow=2
---
source3/lib/ms_fnmatch.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/source3/lib/ms_fnmatch.c b/source3/lib/ms_fnmatch.c
index 9763afefe76..a69407b5267 100644
--- a/source3/lib/ms_fnmatch.c
+++ b/source3/lib/ms_fnmatch.c
@@ -150,7 +150,8 @@ int ms_fnmatch(const char *pattern, const char *string, bool translate_pattern,
{
smb_ucs2_t *p = NULL;
smb_ucs2_t *s = NULL;
- int ret, count, i;
+ int ret;
+ size_t count, i;
struct max_n *max_n = NULL;
struct max_n *max_n_free = NULL;
struct max_n one_max_n;
--
2.16.2
From 5dd569b7c6682c3797dc2d3b9234e5e104177621 Mon Sep 17 00:00:00 2001
Date: Wed, 21 Mar 2018 11:24:45 +0100
Subject: [PATCH 3/6] s3:lib: Fix size types in tldap_find_first_star()
This fixes compilation with -Wstrict-overflow=2
---
source3/lib/tldap.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/source3/lib/tldap.c b/source3/lib/tldap.c
index 205a9cf2b06..bfb24ee8661 100644
--- a/source3/lib/tldap.c
+++ b/source3/lib/tldap.c
@@ -1262,7 +1262,8 @@ static bool tldap_find_first_star(const char *val, const char **star)
static bool tldap_unescape_inplace(char *value, size_t *val_len)
{
- int c, i, p;
+ int c;
+ size_t i, p;
for (i = 0,p = 0; i < *val_len; i++) {
--
2.16.2
From e67e9fa418ba12237f20a724580044352e8581a8 Mon Sep 17 00:00:00 2001
Date: Wed, 21 Mar 2018 11:26:55 +0100
Subject: [PATCH 4/6] lib:param: Fix the size type in
lp_do_parameter_parametric()
This fixes compilation with -Wstrict-overflow=2
---
lib/param/loadparm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/param/loadparm.c b/lib/param/loadparm.c
index b46700dfb54..0c1b28babbc 100644
--- a/lib/param/loadparm.c
+++ b/lib/param/loadparm.c
@@ -1598,7 +1598,7 @@ static bool lp_do_parameter_parametric(struct loadparm_context *lp_ctx,
static bool set_variable_helper(TALLOC_CTX *mem_ctx, int parmnum, void *parm_ptr,
const char *pszParmName, const char *pszParmValue)
{
- int i;
+ size_t i;
/* switch on the type of variable it is */
switch (parm_table[parmnum].type)
--
2.16.2
From 9e581a3a8bac311bbbb927ec63be3ec7ab06148f Mon Sep 17 00:00:00 2001
Date: Wed, 21 Mar 2018 11:55:45 +0100
Subject: [PATCH 5/6] talloc: Fix size type and checks in _vasprintf_tc
This fixes compilation with -Wstrict-overflow=2
---
lib/talloc/talloc.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c
index cd159ef89c2..430ebc70f54 100644
--- a/lib/talloc/talloc.c
+++ b/lib/talloc/talloc.c
@@ -2554,7 +2554,8 @@ static struct talloc_chunk *_vasprintf_tc(const void *t,
const char *fmt,
va_list ap)
{
- int len;
+ int vlen;
+ size_t len;
char *ret;
va_list ap2;
struct talloc_chunk *tc;
@@ -2562,9 +2563,13 @@ static struct talloc_chunk *_vasprintf_tc(const void *t,
/* this call looks strange, but it makes it work on older solaris boxes */
va_copy(ap2, ap);
- len = vsnprintf(buf, sizeof(buf), fmt, ap2);
+ vlen = vsnprintf(buf, sizeof(buf), fmt, ap2);
va_end(ap2);
- if (unlikely(len < 0)) {
+ if (unlikely(vlen < 0)) {
+ return NULL;
+ }
+ len = vlen;
+ if (unlikely(len + 1 < len)) {
return NULL;
}
--
2.16.2
From d41040131077bd611b9ac1ba7f8eec60fcdb729a Mon Sep 17 00:00:00 2001
Date: Thu, 7 Dec 2017 15:27:44 +0100
Subject: [PATCH 6/6] wafsamba: Add '-Werror=strict-overflow
-Wstrict-overflow=2' to the developer build
We could move it to 3, but shouldn't go higher. If you set it to 4 and 5
youl will probably also get a lot of false positives.
---
buildtools/wafsamba/samba_autoconf.py | 2 ++
1 file changed, 2 insertions(+)
diff --git a/buildtools/wafsamba/samba_autoconf.py b/buildtools/wafsamba/samba_autoconf.py
index 35f4f36f61c..bdd7c8bd195 100644
--- a/buildtools/wafsamba/samba_autoconf.py
+++ b/buildtools/wafsamba/samba_autoconf.py
testflags=True)
conf.ADD_CFLAGS('-Wimplicit-fallthrough',
testflags=True)
+ conf.ADD_CFLAGS('-Werror=strict-overflow -Wstrict-overflow=2',
+ testflags=True)
conf.ADD_CFLAGS('-Wformat=2 -Wno-format-y2k', testflags=True)
conf.ADD_CFLAGS('-Wno-format-zero-length', testflags=True)
--
2.16.2
Jeremy Allison via samba-technical
2018-03-21 17:26:04 UTC
Permalink
Post by Jeremy Allison via samba-technical
Post by Andreas Schneider via samba-technical
On Tue, Mar 20, 2018 at 01:18:39PM -0700, Jeremy Allison via samba-technical
Post by Andreas Schneider via samba-technical
This is a ping :-)
Thanks for the review!
Having said that - [PATCH 32/39] lib:param: Fix P_LIST case in
Here are the latest fixes addressing the remaining issues.
:-)
Please check. Thanks!
and pushed. Thanks !
Spoke too soon. On my workstation the build passes,

gcc -v
gcc version 7.3.0 (Debian 7.3.0-5)

but on sn-devel we have:

[ 658/4270] Compiling lib/util/util_file.c
../lib/util/util_file.c: In function ???fgets_slash???:
../lib/util/util_file.c:108:8: error: assuming signed overflow does not occur when simplifying conditional to constant [-Werror=strict-overflow]
while (len < maxlen-1) {
^
../lib/util/util_file.c:108:8: error: assuming signed overflow does not occur when simplifying conditional to constant [-Werror=strict-overflow]
cc1: all warnings being treated as errors

sn-devel-144:~$ gcc -v
gcc version 4.8.4 (Ubuntu 4.8.4-2ubuntu1~14.04.4)

Andreas, I think you're going to have to work this
through on sn-devel first if it's going to go
through.

I've re-pushed without the final patch that
turns on '-Werror=strict-overflow -Wstrict-overflow=2'.
Andrew Bartlett via samba-technical
2018-03-21 17:55:57 UTC
Permalink
On Wed, 2018-03-21 at 10:26 -0700, Jeremy Allison via samba-technical
Post by Jeremy Allison via samba-technical
Spoke too soon. On my workstation the build passes,
gcc -v
gcc version 7.3.0 (Debian 7.3.0-5)
[ 658/4270] Compiling lib/util/util_file.c
../lib/util/util_file.c:108:8: error: assuming signed overflow does not occur when simplifying conditional to constant [-Werror=strict-overflow]
while (len < maxlen-1) {
^
../lib/util/util_file.c:108:8: error: assuming signed overflow does not occur when simplifying conditional to constant [-Werror=strict-overflow]
cc1: all warnings being treated as errors
sn-devel-144:~$ gcc -v
gcc version 4.8.4 (Ubuntu 4.8.4-2ubuntu1~14.04.4)
Andreas, I think you're going to have to work this
through on sn-devel first if it's going to go
through.
Or push to github and have travis-ci chew it over (make a pull request
or set it up on the source repo). Andreas did that (made a pull
request) for the last big set he was working with me and it was great
to have the confidence that the compiler said it was OK.

(Both Travis CI and sn-devel are ubuntu 14.04, so we get the same
compiler behaviour).

Andrew Bartlett
--
Andrew Bartlett http://samba.org/~abartlet/
Authentication Developer, Samba Team http://samba.org
Samba Developer, Catalyst IT http://catalyst.net.nz/services/samba
Jeremy Allison via samba-technical
2018-03-21 18:22:45 UTC
Permalink
Post by Andrew Bartlett via samba-technical
On Wed, 2018-03-21 at 10:26 -0700, Jeremy Allison via samba-technical
Post by Jeremy Allison via samba-technical
Spoke too soon. On my workstation the build passes,
gcc -v
gcc version 7.3.0 (Debian 7.3.0-5)
[ 658/4270] Compiling lib/util/util_file.c
../lib/util/util_file.c:108:8: error: assuming signed overflow does not occur when simplifying conditional to constant [-Werror=strict-overflow]
while (len < maxlen-1) {
^
../lib/util/util_file.c:108:8: error: assuming signed overflow does not occur when simplifying conditional to constant [-Werror=strict-overflow]
cc1: all warnings being treated as errors
sn-devel-144:~$ gcc -v
gcc version 4.8.4 (Ubuntu 4.8.4-2ubuntu1~14.04.4)
Andreas, I think you're going to have to work this
through on sn-devel first if it's going to go
through.
Or push to github and have travis-ci chew it over (make a pull request
or set it up on the source repo). Andreas did that (made a pull
Yes, but that would mean pushing to a proprietary software-as-a-service
provider, which is something we're trying to avoid.

github != Free Software.

We need to remember that. Yes I know I also work for a proprietary
software-as-a-service vendor, but none of our infrastructure *depends*
on it. I'm trying to avoid us drifting into that place by accident.
Andreas Schneider via samba-technical
2018-03-21 18:33:33 UTC
Permalink
Post by Jeremy Allison via samba-technical
Post by Andrew Bartlett via samba-technical
On Wed, 2018-03-21 at 10:26 -0700, Jeremy Allison via samba-technical
Post by Jeremy Allison via samba-technical
Spoke too soon. On my workstation the build passes,
gcc -v
gcc version 7.3.0 (Debian 7.3.0-5)
[ 658/4270] Compiling lib/util/util_file.c
../lib/util/util_file.c:108:8: error: assuming signed overflow does not
occur when simplifying conditional to constant
[-Werror=strict-overflow]> >
while (len < maxlen-1) {
^
../lib/util/util_file.c:108:8: error: assuming signed overflow does not
occur when simplifying conditional to constant
[-Werror=strict-overflow] cc1: all warnings being treated as errors
sn-devel-144:~$ gcc -v
gcc version 4.8.4 (Ubuntu 4.8.4-2ubuntu1~14.04.4)
Andreas, I think you're going to have to work this
through on sn-devel first if it's going to go
through.
Or push to github and have travis-ci chew it over (make a pull request
or set it up on the source repo). Andreas did that (made a pull
Yes, but that would mean pushing to a proprietary software-as-a-service
provider, which is something we're trying to avoid.
github != Free Software.
We need to remember that. Yes I know I also work for a proprietary
software-as-a-service vendor, but none of our infrastructure *depends*
on it. I'm trying to avoid us drifting into that place by accident.
I have a new patchset in preparation with more fixes as autobuild found more
issues then my newer gcc.

However I have a segfault right now I'm trying to spot :-)


Andreas
--
Andreas Schneider GPG-ID: CC014E3D
Samba Team ***@samba.org
www.samba.org
Andreas Schneider via samba-technical
2018-03-21 18:41:54 UTC
Permalink
Post by Jeremy Allison via samba-technical
Post by Andrew Bartlett via samba-technical
On Wed, 2018-03-21 at 10:26 -0700, Jeremy Allison via samba-technical
Post by Jeremy Allison via samba-technical
Spoke too soon. On my workstation the build passes,
gcc -v
gcc version 7.3.0 (Debian 7.3.0-5)
[ 658/4270] Compiling lib/util/util_file.c
../lib/util/util_file.c:108:8: error: assuming signed overflow does not
occur when simplifying conditional to constant
[-Werror=strict-overflow]> >
while (len < maxlen-1) {
^
../lib/util/util_file.c:108:8: error: assuming signed overflow does not
occur when simplifying conditional to constant
[-Werror=strict-overflow] cc1: all warnings being treated as errors
sn-devel-144:~$ gcc -v
gcc version 4.8.4 (Ubuntu 4.8.4-2ubuntu1~14.04.4)
Andreas, I think you're going to have to work this
through on sn-devel first if it's going to go
through.
Or push to github and have travis-ci chew it over (make a pull request
or set it up on the source repo). Andreas did that (made a pull
Yes, but that would mean pushing to a proprietary software-as-a-service
provider, which is something we're trying to avoid.
github != Free Software.
We need to remember that. Yes I know I also work for a proprietary
software-as-a-service vendor, but none of our infrastructure *depends*
on it. I'm trying to avoid us drifting into that place by accident.
Attached is my current version. The ldb fixes don't work. I think this code
needs unit tests and some rethinking. I need to dive into that tomorrow.


Andreas
--
Andreas Schneider GPG-ID: CC014E3D
Samba Team ***@samba.org
www.samba.org
Andrew Bartlett via samba-technical
2018-03-21 21:54:02 UTC
Permalink
Post by Andreas Schneider via samba-technical
Attached is my current version. The ldb fixes don't work. I think this code
needs unit tests and some rethinking. I need to dive into that tomorrow.
I wasn't able to find where the attempted ldb patches are, but I want
to mention that you should be sure to check out 

http://git.catalyst.net.nz/gitweb?p=samba.git;a=shortlog;h=refs/heads/gary-lmdb-merged

As Gary has written a lot of unit tests for ldb to support the lmdb
work, and you may be able to build on those.

Andrew Bartlett
--
Andrew Bartlett
https://samba.org/~abartlet/
Authentication Developer, Samba Team https://samba.org
Samba Development and Support, Catalyst IT
https://catalyst.net.nz/services/samba
Andreas Schneider via samba-technical
2018-03-22 06:31:47 UTC
Permalink
Post by Andrew Bartlett via samba-technical
Post by Andreas Schneider via samba-technical
Attached is my current version. The ldb fixes don't work. I think this code
needs unit tests and some rethinking. I need to dive into that tomorrow.
I wasn't able to find where the attempted ldb patches are, but I want
to mention that you should be sure to check out
http://git.catalyst.net.nz/gitweb?p=samba.git;a=shortlog;h=refs/heads/gary-l
mdb-merged
Thanks, but I don't thing ldb_qsort is covered :-)

I'm talking about this code:

https://git.samba.org/?p=asn/
samba.git;a=commitdiff;h=f64281258f2a5a4c8f85004436ce430b01c9cea7

The patch is HACKISH and I need to write unit tests first and then fix it.



Andreas
--
Andreas Schneider GPG-ID: CC014E3D
Samba Team ***@samba.org
www.samba.org
Andrew Bartlett via samba-technical
2018-03-23 23:37:50 UTC
Permalink
Post by Andreas Schneider via samba-technical
Post by Andrew Bartlett via samba-technical
Post by Andreas Schneider via samba-technical
Attached is my current version. The ldb fixes don't work. I think this code
needs unit tests and some rethinking. I need to dive into that tomorrow.
I wasn't able to find where the attempted ldb patches are, but I want
to mention that you should be sure to check out
http://git.catalyst.net.nz/gitweb?p=samba.git;a=shortlog;h=refs/heads/gary-l
mdb-merged
Thanks, but I don't thing ldb_qsort is covered :-)
https://git.samba.org/?p=asn/
samba.git;a=commitdiff;h=f64281258f2a5a4c8f85004436ce430b01c9cea7
The patch is HACKISH and I need to write unit tests first and then fix it.
Could we abandon it in favour of:

void qsort_r(void *base, size_t nmemb, size_t size,
int (*compar)(const void *, const void *, void *),
void *arg);

on GNU and BSD systems at least?

Perhaps push it to lib/replace for the remaining systems?

Andrew Bartlett
--
Andrew Bartlett http://samba.org/~abartlet/
Authentication Developer, Samba Team http://samba.org
Samba Developer, Catalyst IT http://catalyst.net.nz/services/samba
Timur I. Bakeyev via samba-technical
2018-03-24 01:15:46 UTC
Permalink
On 24 March 2018 at 00:37, Andrew Bartlett via samba-technical <
Post by Andreas Schneider via samba-technical
Post by Andreas Schneider via samba-technical
Post by Andrew Bartlett via samba-technical
Post by Andreas Schneider via samba-technical
Attached is my current version. The ldb fixes don't work. I think
this
Post by Andreas Schneider via samba-technical
Post by Andrew Bartlett via samba-technical
Post by Andreas Schneider via samba-technical
code
needs unit tests and some rethinking. I need to dive into that
tomorrow.
Post by Andreas Schneider via samba-technical
Post by Andrew Bartlett via samba-technical
I wasn't able to find where the attempted ldb patches are, but I want
to mention that you should be sure to check out
http://git.catalyst.net.nz/gitweb?p=samba.git;a=shortlog;h=
refs/heads/gary-l
Post by Andreas Schneider via samba-technical
Post by Andrew Bartlett via samba-technical
mdb-merged
Thanks, but I don't thing ldb_qsort is covered :-)
https://git.samba.org/?p=asn/
samba.git;a=commitdiff;h=f64281258f2a5a4c8f85004436ce430b01c9cea7
The patch is HACKISH and I need to write unit tests first and then fix
it.
void qsort_r(void *base, size_t nmemb, size_t size,
int (*compar)(const void *, const void *, void *),
void *arg);
on GNU and BSD systems at least?
On unrelated note I want to remind everyone that at least for FreeBSD:

The algorithms implemented by qsort(), qsort_r(), and heapsort() are
_not_
stable, that is, if two members compare as equal, their order in the
sorted array is undefined.

We've been bitten by this at least once. So something to keep in mind...

With regards,
Timur Bakeyev.
Douglas Bagnall via samba-technical
2018-03-24 04:53:37 UTC
Permalink
The algorithms implemented by qsort(), qsort_r(), and heapsort() are _not_
stable, that is, if two members compare as equal, their order in the
sorted array is undefined.
We've been bitten by this at least once. So something to keep in mind...
This is actually also true with linux/glibc under memory pressure, where qsort()
et. al. abandon their surreptitious attempts at mergesort:

https://sourceware.org/git/?p=glibc.git;a=blob;f=stdlib/msort.c;h=266c2538c07e86d058359d47388fe21cbfdb525a;hb=refs/heads/master#l213

The documentation explicitly disclaims sort stability.

Douglas
Andreas Schneider via samba-technical
2018-03-24 08:15:36 UTC
Permalink
Post by Andrew Bartlett via samba-technical
Post by Andreas Schneider via samba-technical
Post by Andrew Bartlett via samba-technical
Post by Andreas Schneider via samba-technical
Attached is my current version. The ldb fixes don't work. I think this code
needs unit tests and some rethinking. I need to dive into that tomorrow.
I wasn't able to find where the attempted ldb patches are, but I want
to mention that you should be sure to check out
http://git.catalyst.net.nz/gitweb?p=samba.git;a=shortlog;h=refs/heads/ga
ry-l mdb-merged
Thanks, but I don't thing ldb_qsort is covered :-)
https://git.samba.org/?p=asn/
samba.git;a=commitdiff;h=f64281258f2a5a4c8f85004436ce430b01c9cea7
The patch is HACKISH and I need to write unit tests first and then fix it.
void qsort_r(void *base, size_t nmemb, size_t size,
int (*compar)(const void *, const void *, void *),
void *arg);
on GNU and BSD systems at least?
Perhaps push it to lib/replace for the remaining systems?
Andrew Bartlett
The way the algorithm is implemented isn't really bad, just the code would
need more tests and then some real cleanup. The libc qsort in various
implementations have issues so it would be better to improve this algorithm
and put it in a common place. However this function is public API, we can't
really remove it.



Andreas
Andrew Bartlett via samba-technical
2018-03-28 01:19:37 UTC
Permalink
Post by Andreas Schneider via samba-technical
The way the algorithm is implemented isn't really bad, just the code would
need more tests and then some real cleanup. The libc qsort in various
implementations have issues so it would be better to improve this algorithm
and put it in a common place. However this function is public API, we can't
really remove it.
Well, we can just leave it as a wrapper around qsort_r().  

Our copy was just taken (under licence) from libc anyway.

Andrew Bartlett
--
Andrew Bartlett
https://samba.org/~abartlet/
Authentication Developer, Samba Team https://samba.org
Samba Development and Support, Catalyst IT
https://catalyst.net.nz/services/samba
Andrew Bartlett via samba-technical
2018-03-21 18:48:03 UTC
Permalink
Post by Jeremy Allison via samba-technical
Post by Andrew Bartlett via samba-technical
Or push to github and have travis-ci chew it over (make a pull request
or set it up on the source repo). Andreas did that (made a pull
Yes, but that would mean pushing to a proprietary software-as-a-service
provider, which is something we're trying to avoid.
Except for Coverity of course.
Post by Jeremy Allison via samba-technical
github != Free Software.
Sure. And to be clear, as someone who actively avoids having Google
accounts I really do get that.
Post by Jeremy Allison via samba-technical
We need to remember that. Yes I know I also work for a proprietary
software-as-a-service vendor, but none of our infrastructure *depends*
on it. I'm trying to avoid us drifting into that place by accident.
Will you help build and maintain an alternative? Or at least can you
commit to using it?

Ideally I would ask that you use the current GitHub integration, and
tell me what you like and what doesn't work and where it impinges on
your freedoms.

That would give folks like me actively working in this area solid data.
I wouldn't want to build something 'perfect' and then find that the
real issue was 'actually, I really just don't like the e-mails' ;-).

(The irony is that the clunky e-mails exist entirely because we didn't
want to either split the community nor mandate folks sign in to
GitHub).

There are alternatives, but also precious as our freedom is our time,
and time I spend manually compile-testing patches is time I can't spend
on developing more Free Software.

Andrew Bartlett
--
Andrew Bartlett http://samba.org/~abartlet/
Authentication Developer, Samba Team http://samba.org
Samba Developer, Catalyst IT http://catalyst.net.nz/services/samba
Jeremy Allison via samba-technical
2018-03-21 18:56:55 UTC
Permalink
Post by Andrew Bartlett via samba-technical
Post by Jeremy Allison via samba-technical
Post by Andrew Bartlett via samba-technical
Or push to github and have travis-ci chew it over (make a pull request
or set it up on the source repo). Andreas did that (made a pull
Yes, but that would mean pushing to a proprietary software-as-a-service
provider, which is something we're trying to avoid.
Except for Coverity of course.
No, there is a difference. Coverity and Codenomicon are *extras*.
Nice to have but not an essential part of our workflow.

Github is trying to become an essential part of our workflow.
Post by Andrew Bartlett via samba-technical
Post by Jeremy Allison via samba-technical
github != Free Software.
Sure. And to be clear, as someone who actively avoids having Google
accounts I really do get that.
Post by Jeremy Allison via samba-technical
We need to remember that. Yes I know I also work for a proprietary
software-as-a-service vendor, but none of our infrastructure *depends*
on it. I'm trying to avoid us drifting into that place by accident.
Will you help build and maintain an alternative? Or at least can you
commit to using it?
I can't commit to using something I don't know right now, but I
do want the features that we get from github as part of our workflow,
just not depending on github.

github could get bought out/go away/add privacy issues/add service
charges *tomorrow*, and when they do (it's a when, not an if), then
a lot of projects are going to be *screwed*. I don't want to be one
of them.
Post by Andrew Bartlett via samba-technical
Ideally I would ask that you use the current GitHub integration, and
tell me what you like and what doesn't work and where it impinges on
your freedoms.
That would give folks like me actively working in this area solid data.
I wouldn't want to build something 'perfect' and then find that the
real issue was 'actually, I really just don't like the e-mails' ;-).
(The irony is that the clunky e-mails exist entirely because we didn't
want to either split the community nor mandate folks sign in to
GitHub).
There are alternatives, but also precious as our freedom is our time,
and time I spend manually compile-testing patches is time I can't spend
on developing more Free Software.
I understand convenience. I'm as guilty of it as anyone. But
I don't want to *depend* on github such that it's not possible
to build Samba without it.

Jeremy.
Andrew Bartlett via samba-technical
2018-03-21 22:06:04 UTC
Permalink
Post by Jeremy Allison via samba-technical
Post by Andrew Bartlett via samba-technical
Post by Jeremy Allison via samba-technical
Post by Andrew Bartlett via samba-technical
Or push to github and have travis-ci chew it over (make a pull request
or set it up on the source repo). Andreas did that (made a pull
Yes, but that would mean pushing to a proprietary software-as-a-service
provider, which is something we're trying to avoid.
Except for Coverity of course.
No, there is a difference. Coverity and Codenomicon are *extras*.
Nice to have but not an essential part of our workflow.
Github is trying to become an essential part of our workflow.
I'm not proposing that, and it isn't. A while back we agreed to
welcome contributions via GitHub. As discussed elsewhere it helps a
lot because the CI is pre-done and so avoids round-trips with new
contributors who can't run our full 5 hour autobuild locally.

The only change I see in the future is that I would like it to become
socially unacceptable to proposed changes to Samba that don't pass CI. 

I don't really mind which CI is used, GitHub for those in that
ecosystem, GitLab if I get help to get that going, Catalyst Cloud for
Catalyst staff or sn-devel for team members like Metze and Volker who
use that religiously. 

If, like the introduction of autobuild itself, CI is easy to do and it
becomes unacceptable to propose breaking changes, we will *all* spend
less time on admin and e-mail and more time reviewing and writing great
patches.
Post by Jeremy Allison via samba-technical
Post by Andrew Bartlett via samba-technical
Post by Jeremy Allison via samba-technical
github != Free Software.
Sure. And to be clear, as someone who actively avoids having Google
accounts I really do get that.
github could get bought out/go away/add privacy issues/add service
charges *tomorrow*, and when they do (it's a when, not an if), then
a lot of projects are going to be *screwed*. I don't want to be one
of them.
I credit the Open Source and Free Software communities with far more
agility than that. We have survived the loss of sourceforge, Google
Code, berlios, Fedora Hosted and many others without major pain.

And all our pull requests are mailed as patches, so we won't even loose
those.
Post by Jeremy Allison via samba-technical
Post by Andrew Bartlett via samba-technical
There are alternatives, but also precious as our freedom is our time,
and time I spend manually compile-testing patches is time I can't spend
on developing more Free Software.
I understand convenience. I'm as guilty of it as anyone. But
I don't want to *depend* on github such that it's not possible
to build Samba without it.
I'm not proposing that, and wouldn't. As a good example we need a way
to do security releases in private regardless.

But what I will do is ask folks for the CI status before I do reviews,
because my time is valuable too.

Thanks,

Andrew Bartlett
--
Andrew Bartlett
https://samba.org/~abartlet/
Authentication Developer, Samba Team https://samba.org
Samba Development and Support, Catalyst IT
https://catalyst.net.nz/services/samba
Volker Lendecke via samba-technical
2018-03-21 19:04:59 UTC
Permalink
Post by Andrew Bartlett via samba-technical
Post by Jeremy Allison via samba-technical
Post by Andrew Bartlett via samba-technical
Or push to github and have travis-ci chew it over (make a pull request
or set it up on the source repo). Andreas did that (made a pull
Yes, but that would mean pushing to a proprietary software-as-a-service
provider, which is something we're trying to avoid.
Except for Coverity of course.
Coverity is no more. The site has been in "planned maintenance" for
weeks now. I don't think it will come back.

Volker
--
Besuchen Sie die verinice.XP 2018 in Berlin,
Anwenderkonferenz für Informationssicherheit
vom 21.-23.03.2018 im Sofitel Kurfürstendamm
Info & Anmeldung hier: http://veriniceXP.org

SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:***@sernet.de
Jeremy Allison via samba-technical
2018-03-21 19:15:36 UTC
Permalink
Post by Volker Lendecke via samba-technical
Post by Andrew Bartlett via samba-technical
Post by Jeremy Allison via samba-technical
Post by Andrew Bartlett via samba-technical
Or push to github and have travis-ci chew it over (make a pull request
or set it up on the source repo). Andreas did that (made a pull
Yes, but that would mean pushing to a proprietary software-as-a-service
provider, which is something we're trying to avoid.
Except for Coverity of course.
Coverity is no more. The site has been in "planned maintenance" for
weeks now. I don't think it will come back.
They got hacked to mine crypto-currency. They should be back now.

https://www.theregister.co.uk/2018/03/19/coverity_scan_cryptomining/

Crypto-currency will be the heat-death of the planet. And we will
all deserve it :-).
Volker Lendecke via samba-technical
2018-03-21 19:31:28 UTC
Permalink
Post by Jeremy Allison via samba-technical
They got hacked to mine crypto-currency. They should be back now.
Nope. Now they time out. They're dead as in "they won't come back".
It was too good to be true to get "free" service. There ain't anything
really free with big corps. They will pull the plug whenever they
want.

Volker
Martin Schwenke via samba-technical
2018-03-21 19:43:54 UTC
Permalink
On Wed, 21 Mar 2018 20:31:28 +0100, Volker Lendecke via samba-technical
Post by Volker Lendecke via samba-technical
Post by Jeremy Allison via samba-technical
They got hacked to mine crypto-currency. They should be back now.
Nope. Now they time out. They're dead as in "they won't come back".
It was too good to be true to get "free" service. There ain't anything
really free with big corps. They will pull the plug whenever they
want.
The web site is working "like a bought one" (Australian expression,
assumes things you pay for are always better ;-) from here...

peace & happiness,
martin
Volker Lendecke via samba-technical
2018-03-27 23:33:31 UTC
Permalink
Post by Volker Lendecke via samba-technical
Post by Jeremy Allison via samba-technical
They got hacked to mine crypto-currency. They should be back now.
Nope. Now they time out. They're dead as in "they won't come back".
It was too good to be true to get "free" service. There ain't anything
really free with big corps. They will pull the plug whenever they
want.
I stand corrected: They're back.

Volker
--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:***@sernet.de
Andreas Schneider via samba-technical
2018-03-22 20:48:00 UTC
Permalink
On Wednesday, 21 March 2018 18:26:04 CET Jeremy Allison via samba-technical
Post by Jeremy Allison via samba-technical
Andreas, I think you're going to have to work this
through on sn-devel first if it's going to go
through.
I've re-pushed without the final patch that
turns on '-Werror=strict-overflow -Wstrict-overflow=2'.
I'm still working on this and the gcc on sn-devel is much more picky than my
gcc.

I'm attaching the latest patchset which still does not fully pass autobuild.
The current issue is that ancient getdate.y/getdate.c for vfs_readonly. I
wondered if running flex getdate.y would maybe produce and up to date version
which is compiler friendlier but I have no clue how this stuff really works.

Alexander?


However I've fixed the ldb_qsort issue and wrote a test for it.


Cheers,


Andreas
--
Andreas Schneider GPG-ID: CC014E3D
Samba Team ***@samba.org
www.samba.org
Alexander Bokovoy via samba-technical
2018-03-22 21:13:44 UTC
Permalink
Post by Andreas Schneider via samba-technical
On Wednesday, 21 March 2018 18:26:04 CET Jeremy Allison via samba-technical
Post by Jeremy Allison via samba-technical
Andreas, I think you're going to have to work this
through on sn-devel first if it's going to go
through.
I've re-pushed without the final patch that
turns on '-Werror=strict-overflow -Wstrict-overflow=2'.
I'm still working on this and the gcc on sn-devel is much more picky than my
gcc.
I'm attaching the latest patchset which still does not fully pass autobuild.
The current issue is that ancient getdate.y/getdate.c for vfs_readonly. I
wondered if running flex getdate.y would maybe produce and up to date version
which is compiler friendlier but I have no clue how this stuff really works.
Alexander?
You can rebuild getdate.c with bison: bison -o getdate.c getdate.y
I tried rebuilding with F27's bison (3.0.4) and the only issue it found
was %pure_parser -- deprecated, should be %pure-parser (- versus _).

The generated getdate.c seems to be reasonable but a bit different in
how code is arranged. It needs to be tested more.
--
/ Alexander Bokovoy
Andreas Schneider via samba-technical
2018-03-23 14:36:00 UTC
Permalink
Post by Jeremy Allison via samba-technical
Andreas, I think you're going to have to work this
through on sn-devel first if it's going to go
through.
OK, the attached patchset builds on sn-devel (gcc is more picky there) and a
private autobuild is currently running and more than 60% of the tests already
passed.

The ldb_qsort was tricky so I wrote a test for it.

I've set '-Wstrict-overflow=1' for bison generated getdate.c which uses C89
and our old nmbd code.


Could you please review the remaining patches?



Thanks,


Andreas
--
Andreas Schneider GPG-ID: CC014E3D
Samba Team ***@samba.org
www.samba.org
Stefan Metzmacher via samba-technical
2018-03-23 14:56:24 UTC
Permalink
Post by Andreas Schneider via samba-technical
Post by Jeremy Allison via samba-technical
Andreas, I think you're going to have to work this
through on sn-devel first if it's going to go
through.
OK, the attached patchset builds on sn-devel (gcc is more picky there) and a
private autobuild is currently running and more than 60% of the tests already
passed.
The ldb_qsort was tricky so I wrote a test for it.
I've set '-Wstrict-overflow=1' for bison generated getdate.c which uses C89
and our old nmbd code.
We can't add a hardcoded "-Wstrict-overflow=1" to every build.

We may have to just add -Wstrict-overflow=2 to PICKY_FLAGS,
so that this isn't added with 'allow_warnings=True'.

That would be a huge advantage already and makes sure we don't regress
in most of our code.

metze
Andreas Schneider via samba-technical
2018-03-23 15:32:19 UTC
Permalink
Post by Stefan Metzmacher via samba-technical
Post by Andreas Schneider via samba-technical
Post by Jeremy Allison via samba-technical
Andreas, I think you're going to have to work this
through on sn-devel first if it's going to go
through.
OK, the attached patchset builds on sn-devel (gcc is more picky there) and
a private autobuild is currently running and more than 60% of the tests
already passed.
The ldb_qsort was tricky so I wrote a test for it.
I've set '-Wstrict-overflow=1' for bison generated getdate.c which uses C89
and our old nmbd code.
We can't add a hardcoded "-Wstrict-overflow=1" to every build.
We may have to just add -Wstrict-overflow=2 to PICKY_FLAGS,
so that this isn't added with 'allow_warnings=True'.
That would be a huge advantage already and makes sure we don't regress
in most of our code.
Then we could do it like HAVE_WNO_UNUSED_FUNCTION and turn the warning off.
See attached.


Andreas
--
Andreas Schneider GPG-ID: CC014E3D
Samba Team ***@samba.org
www.samba.org
Stefan Metzmacher via samba-technical
2018-03-23 15:41:34 UTC
Permalink
Post by Andreas Schneider via samba-technical
Post by Stefan Metzmacher via samba-technical
Post by Andreas Schneider via samba-technical
Post by Jeremy Allison via samba-technical
Andreas, I think you're going to have to work this
through on sn-devel first if it's going to go
through.
OK, the attached patchset builds on sn-devel (gcc is more picky there) and
a private autobuild is currently running and more than 60% of the tests
already passed.
The ldb_qsort was tricky so I wrote a test for it.
I've set '-Wstrict-overflow=1' for bison generated getdate.c which uses C89
and our old nmbd code.
We can't add a hardcoded "-Wstrict-overflow=1" to every build.
We may have to just add -Wstrict-overflow=2 to PICKY_FLAGS,
so that this isn't added with 'allow_warnings=True'.
That would be a huge advantage already and makes sure we don't regress
in most of our code.
Then we could do it like HAVE_WNO_UNUSED_FUNCTION and turn the warning off.
See attached.
Thanks! That's much better.

metze
Andreas Schneider via samba-technical
2018-03-27 15:05:09 UTC
Permalink
On Friday, 23 March 2018 16:41:34 CEST Stefan Metzmacher via samba-technical
Post by Stefan Metzmacher via samba-technical
Thanks! That's much better.
Attached is an updated version as we released pam_wrapper 1.0.6.

A private autobuild passed with the patchset already.


Please review. Thanks!


Andreas
--
Andreas Schneider GPG-ID: CC014E3D
Samba Team ***@samba.org
www.samba.org
Jeremy Allison via samba-technical
2018-04-02 23:58:27 UTC
Permalink
Post by Andreas Schneider via samba-technical
On Friday, 23 March 2018 16:41:34 CEST Stefan Metzmacher via samba-technical
Post by Stefan Metzmacher via samba-technical
Thanks! That's much better.
Attached is an updated version as we released pam_wrapper 1.0.6.
A private autobuild passed with the patchset already.
Please review. Thanks!
A quick question on these - why do you have the patch ?

s3:nmbd: Set -Wno-strict-overflow for nmbd if supported

diff --git a/source3/nmbd/wscript_build b/source3/nmbd/wscript_build
index 843c900a4ba..c34deae6083 100644
--- a/source3/nmbd/wscript_build
+++ b/source3/nmbd/wscript_build
@@ -1,5 +1,9 @@
#!/usr/bin/env python

+nmbd_cflags=''
+if bld.CONFIG_SET('HAVE_WNO_STRICT_OVERFLOW'):
+ nmbd_cflags='-Wno-strict-overflow'
+
bld.SAMBA3_BINARY('nmbd',
source='''
asyncdns.c
@@ -30,6 +34,7 @@ bld.SAMBA3_BINARY('nmbd',
nmbd_workgroupdb.c
nmbd_synclists.c
''',
+ cflags_end=nmbd_cflags,
deps='''
talloc
tevent


If I remove this I still get a clean build (at least
here). Just checking before I finish review.

Cheers,

Jeremy.
Andreas Schneider via samba-technical
2018-04-03 06:57:51 UTC
Permalink
On Tue, Mar 27, 2018 at 05:05:09PM +0200, Andreas Schneider via samba-
Post by Andreas Schneider via samba-technical
On Friday, 23 March 2018 16:41:34 CEST Stefan Metzmacher via
samba-technical>
Post by Stefan Metzmacher via samba-technical
Thanks! That's much better.
Attached is an updated version as we released pam_wrapper 1.0.6.
A private autobuild passed with the patchset already.
Please review. Thanks!
A quick question on these - why do you have the patch ?
s3:nmbd: Set -Wno-strict-overflow for nmbd if supported
diff --git a/source3/nmbd/wscript_build b/source3/nmbd/wscript_build
index 843c900a4ba..c34deae6083 100644
--- a/source3/nmbd/wscript_build
+++ b/source3/nmbd/wscript_build
@@ -1,5 +1,9 @@
#!/usr/bin/env python
+nmbd_cflags=''
+ nmbd_cflags='-Wno-strict-overflow'
+
bld.SAMBA3_BINARY('nmbd',
source='''
asyncdns.c
@@ -30,6 +34,7 @@ bld.SAMBA3_BINARY('nmbd',
nmbd_workgroupdb.c
nmbd_synclists.c
''',
+ cflags_end=nmbd_cflags,
deps='''
talloc
tevent
If I remove this I still get a clean build (at least
here). Just checking before I finish review.
I did get a clean build on my machine too, but autobuild (samba-o3) complained
about it.

As autobuild got updated recently, it doesn't complain about this anymore,
just tested in a private build on sn-devel.


Cheers,

Andreas
--
Andreas Schneider GPG-ID: CC014E3D
Samba Team ***@samba.org
www.samba.org
Andreas Schneider via samba-technical
2018-04-03 12:02:53 UTC
Permalink
On Tue, Mar 27, 2018 at 05:05:09PM +0200, Andreas Schneider via samba-
Post by Andreas Schneider via samba-technical
On Friday, 23 March 2018 16:41:34 CEST Stefan Metzmacher via
samba-technical>
Post by Stefan Metzmacher via samba-technical
Thanks! That's much better.
Attached is an updated version as we released pam_wrapper 1.0.6.
A private autobuild passed with the patchset already.
Please review. Thanks!
A quick question on these - why do you have the patch ?
s3:nmbd: Set -Wno-strict-overflow for nmbd if supported
diff --git a/source3/nmbd/wscript_build b/source3/nmbd/wscript_build
index 843c900a4ba..c34deae6083 100644
--- a/source3/nmbd/wscript_build
+++ b/source3/nmbd/wscript_build
@@ -1,5 +1,9 @@
#!/usr/bin/env python
+nmbd_cflags=''
+ nmbd_cflags='-Wno-strict-overflow'
+
bld.SAMBA3_BINARY('nmbd',
source='''
asyncdns.c
@@ -30,6 +34,7 @@ bld.SAMBA3_BINARY('nmbd',
nmbd_workgroupdb.c
nmbd_synclists.c
''',
+ cflags_end=nmbd_cflags,
deps='''
talloc
tevent
If I remove this I still get a clean build (at least
here). Just checking before I finish review.
Ah, sorry my bad. I didn't have the tree with the reverted patch. Here we go:


../source3/nmbd/nmbd_incomingrequests.c: In function
‘process_node_status_request’:
../source3/nmbd/nmbd_incomingrequests.c:344:8: error: assuming pointer
wraparound does not occur when comparing P +- C1 with P +- C2 [-Werror=strict-
overflow]
while (buf < bufend) {
^
cc1: all warnings being treated as errors




Andreas
--
Andreas Schneider GPG-ID: CC014E3D
Samba Team ***@samba.org
www.samba.org
Jeremy Allison via samba-technical
2018-04-03 15:45:25 UTC
Permalink
Post by Andreas Schneider via samba-technical
../source3/nmbd/nmbd_incomingrequests.c: In function
../source3/nmbd/nmbd_incomingrequests.c:344:8: error: assuming pointer
wraparound does not occur when comparing P +- C1 with P +- C2 [-Werror=strict-
overflow]
while (buf < bufend) {
^
cc1: all warnings being treated as errors
Ah, I'm not seeing that. What version of gcc are you using ?

Are there a lot of these in nmbd ? Is it worth fixing nmbd
first, or set to ignore now and work on them later ?

Jeremy.
Andreas Schneider via samba-technical
2018-04-03 16:10:29 UTC
Permalink
Post by Jeremy Allison via samba-technical
Post by Andreas Schneider via samba-technical
../source3/nmbd/nmbd_incomingrequests.c: In function
../source3/nmbd/nmbd_incomingrequests.c:344:8: error: assuming pointer
wraparound does not occur when comparing P +- C1 with P +- C2
[-Werror=strict- overflow]
while (buf < bufend) {
^
cc1: all warnings being treated as errors
Ah, I'm not seeing that. What version of gcc are you using ?
I'm not seeing them either with gcc 7.3.1 on my machines!


I was only able to reproduce this on sn-devel (autobuild) which uses:

gcc-4.8.real (Ubuntu 4.8.4-2ubuntu1~14.04.4) 4.8.4
Post by Jeremy Allison via samba-technical
Are there a lot of these in nmbd ? Is it worth fixing nmbd
first, or set to ignore now and work on them later ?
I don't know. It might be after fixing that one, that it will find more in
that file.

However fixing this one will probably a bit tricky. The optimizer doesn't like
the pointer arithmetic. So you need to use array indices to do the overflow
calculations. By fixing that we would allow the optimizer to understand the
limitations better.

It is possible that gcc 7 is cleverer than 4.8 in this regard and can deal
with the pointer arithmetic here.



I hope that helps.


Andreas
--
Andreas Schneider GPG-ID: CC014E3D
Samba Team ***@samba.org
www.samba.org
Jeremy Allison via samba-technical
2018-04-03 17:23:58 UTC
Permalink
Post by Andreas Schneider via samba-technical
Post by Jeremy Allison via samba-technical
Post by Andreas Schneider via samba-technical
../source3/nmbd/nmbd_incomingrequests.c: In function
../source3/nmbd/nmbd_incomingrequests.c:344:8: error: assuming pointer
wraparound does not occur when comparing P +- C1 with P +- C2
[-Werror=strict- overflow]
while (buf < bufend) {
^
cc1: all warnings being treated as errors
Ah, I'm not seeing that. What version of gcc are you using ?
I'm not seeing them either with gcc 7.3.1 on my machines!
gcc-4.8.real (Ubuntu 4.8.4-2ubuntu1~14.04.4) 4.8.4
Post by Jeremy Allison via samba-technical
Are there a lot of these in nmbd ? Is it worth fixing nmbd
first, or set to ignore now and work on them later ?
I don't know. It might be after fixing that one, that it will find more in
that file.
However fixing this one will probably a bit tricky. The optimizer doesn't like
the pointer arithmetic. So you need to use array indices to do the overflow
calculations. By fixing that we would allow the optimizer to understand the
limitations better.
It is possible that gcc 7 is cleverer than 4.8 in this regard and can deal
with the pointer arithmetic here.
I hope that helps.
Sure does. RB+ and pushed all except the last two patches
that turn on the compiler shitches (or off in the nmbd
case :-).

I would like to get Metze's buy-off before pressing the
final big red button (tm) :-).

Jeremy.
Stefan Metzmacher via samba-technical
2018-04-03 21:49:53 UTC
Permalink
Post by Jeremy Allison via samba-technical
Post by Andreas Schneider via samba-technical
Post by Jeremy Allison via samba-technical
Post by Andreas Schneider via samba-technical
../source3/nmbd/nmbd_incomingrequests.c: In function
../source3/nmbd/nmbd_incomingrequests.c:344:8: error: assuming pointer
wraparound does not occur when comparing P +- C1 with P +- C2
[-Werror=strict- overflow]
while (buf < bufend) {
^
cc1: all warnings being treated as errors
Ah, I'm not seeing that. What version of gcc are you using ?
I'm not seeing them either with gcc 7.3.1 on my machines!
gcc-4.8.real (Ubuntu 4.8.4-2ubuntu1~14.04.4) 4.8.4
Post by Jeremy Allison via samba-technical
Are there a lot of these in nmbd ? Is it worth fixing nmbd
first, or set to ignore now and work on them later ?
I don't know. It might be after fixing that one, that it will find more in
that file.
However fixing this one will probably a bit tricky. The optimizer doesn't like
the pointer arithmetic. So you need to use array indices to do the overflow
calculations. By fixing that we would allow the optimizer to understand the
limitations better.
It is possible that gcc 7 is cleverer than 4.8 in this regard and can deal
with the pointer arithmetic here.
I hope that helps.
Sure does. RB+ and pushed all except the last two patches
that turn on the compiler shitches (or off in the nmbd
case :-).
I would like to get Metze's buy-off before pressing the
final big red button (tm) :-).
I would prefer that we fix the warning, this seems to do the trick
with gcc 4.8:

diff --git a/source3/nmbd/nmbd_incomingrequests.c
b/source3/nmbd/nmbd_incomingrequests.c
index 6f3eee3..96cb124 100644
--- a/source3/nmbd/nmbd_incomingrequests.c
+++ b/source3/nmbd/nmbd_incomingrequests.c
@@ -341,7 +341,7 @@ subnet %s - name not found.\n",
nmb_namestr(&nmb->question.question_name),

namerec = subrec->namelist;

- while (buf < bufend) {
+ while (PTR_DIFF(bufend, buf) > 0) {
if( (namerec->data.source == SELF_NAME) ||
(namerec->data.source == PERMANENT_NAME) ) {
int name_type = namerec->name.name_type;
unstring name;


Can you double check it doesn't change the logic?

But process_node_status_request() is a complete mess and should be
rewritten by someone who understands it:-)

metze
Jeremy Allison via samba-technical
2018-04-03 21:55:13 UTC
Permalink
Post by Stefan Metzmacher via samba-technical
Post by Jeremy Allison via samba-technical
Sure does. RB+ and pushed all except the last two patches
that turn on the compiler shitches (or off in the nmbd
case :-).
I would like to get Metze's buy-off before pressing the
final big red button (tm) :-).
I would prefer that we fix the warning, this seems to do the trick
diff --git a/source3/nmbd/nmbd_incomingrequests.c
b/source3/nmbd/nmbd_incomingrequests.c
index 6f3eee3..96cb124 100644
--- a/source3/nmbd/nmbd_incomingrequests.c
+++ b/source3/nmbd/nmbd_incomingrequests.c
@@ -341,7 +341,7 @@ subnet %s - name not found.\n",
nmb_namestr(&nmb->question.question_name),
namerec = subrec->namelist;
- while (buf < bufend) {
+ while (PTR_DIFF(bufend, buf) > 0) {
if( (namerec->data.source == SELF_NAME) ||
(namerec->data.source == PERMANENT_NAME) ) {
int name_type = namerec->name.name_type;
unstring name;
Can you double check it doesn't change the logic?
Will do.
Post by Stefan Metzmacher via samba-technical
But process_node_status_request() is a complete mess and should be
rewritten by someone who understands it:-)
Have a heart Metze, that's some very old code :-).
Stefan Metzmacher via samba-technical
2018-04-03 21:59:28 UTC
Permalink
Post by Jeremy Allison via samba-technical
Post by Stefan Metzmacher via samba-technical
Post by Jeremy Allison via samba-technical
Sure does. RB+ and pushed all except the last two patches
that turn on the compiler shitches (or off in the nmbd
case :-).
I would like to get Metze's buy-off before pressing the
final big red button (tm) :-).
I would prefer that we fix the warning, this seems to do the trick
diff --git a/source3/nmbd/nmbd_incomingrequests.c
b/source3/nmbd/nmbd_incomingrequests.c
index 6f3eee3..96cb124 100644
--- a/source3/nmbd/nmbd_incomingrequests.c
+++ b/source3/nmbd/nmbd_incomingrequests.c
@@ -341,7 +341,7 @@ subnet %s - name not found.\n",
nmb_namestr(&nmb->question.question_name),
namerec = subrec->namelist;
- while (buf < bufend) {
+ while (PTR_DIFF(bufend, buf) > 0) {
if( (namerec->data.source == SELF_NAME) ||
(namerec->data.source == PERMANENT_NAME) ) {
int name_type = namerec->name.name_type;
unstring name;
Can you double check it doesn't change the logic?
Will do.
Thanks!
Post by Jeremy Allison via samba-technical
Post by Stefan Metzmacher via samba-technical
But process_node_status_request() is a complete mess and should be
rewritten by someone who understands it:-)
Have a heart Metze, that's some very old code :-).
It's not required, just nice to have :-)

metze
Douglas Bagnall via samba-technical
2018-05-04 04:29:34 UTC
Permalink
Post by Andreas Schneider via samba-technical
Hello,
in a lot of places we use the incorrect size types, which could lead to
issues. This is mostly int vs. size_t.
-Werror=strict-overflow -Wstrict-overflow=2
this allows us to use enforce the correct size types. We could discuss moving
to -Wstrict-overflow=3 but higher values don't make sense. You also get a
false positives with 4 or 5.
Since this, I get errors running autobuild on Ubuntu 16.04:

../source4/heimdal/lib/hcrypto/libtommath/bn_mp_rshd.c:55:5: error: assuming signed overflow does not occur when simplifying conditional to constant [-Werror=strict-overflow]
for (x = 0; x < (a->used - b); x++) {
^

and Centos 6:

[ 632/4144] Compiling lib/replace/test/os2_delete.c
../lib/replace/test/os2_delete.c: In function ‘os2_delete’:
../lib/replace/test/os2_delete.c:77: error: assuming signed overflow does not occur when simplifying conditional to constant
cc1: warnings being treated as errors


As far as I can tell, the centos-6 one is gcc 4.4 being blindingly stupid.
The 16.04 one would be avoided if -Werror turned a blind eye to Heimdal,
which I thought already happened.

I am not fixing these with any urgency.

cheers,
Douglas

Loading...