[PATCH] Enforce strict overflow checking
Jeremy Allison
jra at samba.org
Wed Mar 21 17:12:37 UTC 2018
On Wed, Mar 21, 2018 at 12:21:16PM +0100, Andreas Schneider wrote:
> On Tuesday, 20 March 2018 22:38:44 CET Jeremy Allison wrote:
> > On Tue, Mar 20, 2018 at 01:18:39PM -0700, Jeremy Allison via samba-technical
> wrote:
> > > On Thu, Mar 15, 2018 at 10:16:11AM +0100, Andreas Schneider wrote:
> > > > This is a ping :-)
> > > >
> > > >
> > > > Thanks for the review!
> > >
> > > OK, the only one I'm pushing back on is this:
> > Having said that - [PATCH 32/39] lib:param: Fix P_LIST case in
> > set_variable_helper() causes:
>
> Here are the latest fixes addressing the remaining issues.
>
> :-)
>
>
> Please check. Thanks!
Reviewed-by: Jeremy Allison <jra at samba.org>
and pushed. Thanks !
Jeremy.
> --
> Andreas Schneider GPG-ID: CC014E3D
> Samba Team asn at samba.org
> www.samba.org
> From a1df5b7472c017e855e161b843c12c56efbfb947 Mon Sep 17 00:00:00 2001
> From: Andreas Schneider <asn at samba.org>
> 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
>
> Signed-off-by: Jeremy Allison <jra at samba.org>
> Reviewed-by: Andreas Schneider <asn at samba.org>
> ---
> 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
> From: Andreas Schneider <asn at samba.org>
> 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
>
> Signed-off-by: Andreas Schneider <asn at samba.org>
> ---
> 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
> From: Andreas Schneider <asn at samba.org>
> 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
>
> Signed-off-by: Andreas Schneider <asn at samba.org>
> ---
> 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
> From: Andreas Schneider <asn at samba.org>
> 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
>
> Signed-off-by: Andreas Schneider <asn at samba.org>
> ---
> 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
> From: Andreas Schneider <asn at samba.org>
> 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
>
> Signed-off-by: Andreas Schneider <asn at samba.org>
> ---
> 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
> From: Andreas Schneider <asn at samba.org>
> 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.
>
> Signed-off-by: Andreas Schneider <asn at samba.org>
> ---
> 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
> @@ -713,6 +713,8 @@ def SAMBA_CONFIG_H(conf, path=None):
> 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
>
More information about the samba-technical
mailing list