[PATCH] Enforce strict overflow checking

[Jeremy Allison]

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:

--------------------------------------------------------------------------------
From: Andreas Schneider <asn at 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 <asn at 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-s3-printing-Fix-size-check-in-get_file_version.patch
Type: text/x-diff
Size: 1847 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20180320/133dad54/0001-s3-printing-Fix-size-check-in-get_file_version.diff>