[1/3] config: work around gcc-10 -Wstringop-overflow warning
diff mbox series

Message ID 20200804074353.GA284046@coredump.intra.peff.net
State New
Headers show
Series
  • some compiler/asan/ubsan fixes
Related show

Commit Message

Jeff King Aug. 4, 2020, 7:43 a.m. UTC
Compiling with gcc-10, -O2, and -fsanitize=undefined results in a
compiler warning:

  config.c: In function ‘git_config_copy_or_rename_section_in_file’:
  config.c:3170:17: error: writing 1 byte into a region of size 0 [-Werror=stringop-overflow=]
   3170 |       output[0] = '\t';
        |       ~~~~~~~~~~^~~~~~
  config.c:3076:7: note: at offset -1 to object ‘buf’ with size 1024 declared here
   3076 |  char buf[1024];
        |       ^~~

This is a false positive. The interesting lines of code are:

  int i;
  char *output = buf;
  ...
  for (i = 0; buf[i] && isspace(buf[i]); i++)
          ; /* do nothing */
  ...
  int offset;
  offset = section_name_match(&buf[i], old_name);
  if (offset > 0) {
          ...
          output += offset + i;
          if (strlen(output) > 0) {
		  /*
		   * More content means there's
		   * a declaration to put on the
		   * next line; indent with a
		   * tab
		   */
		  output -= 1;
		  output[0] = '\t';
	  }
  }

So we do assign output to buf initially. Later we increment it based on
"offset" and "i" and then subtract "1" from it. That latter step is what
the compiler is complaining about; it could lead to going off the left
side of the array if "output == buf" at the moment of the subtraction.
For that to be the case, then "offset + i" would have to be 0. But that
can't happen:

  - we know that "offset" is at least 1, since we're in a conditional
    block that checks that

  - we know that "i" is not negative, since it started at 0 and only
    incremented over whitespace

So the sum must be at least 1, and therefore it's OK to subtract one
from "output".

But that's not quite the whole story. Since "i" is an int, it could in
theory be possible to overflow to negative (when counting whitespace on
a very large string). But we know that's impossible because we're
counting the 1024-byte buffer we just fed to fgets(), so it can never be
larger than that.

Switching the type of "i" to "unsigned" makes the warning go away, so
let's do that.

Arguably size_t is an even better type (for this and for the other
length fields), but switching to it produces a similar but distinct
warning:

  config.c: In function ‘git_config_copy_or_rename_section_in_file’:
  config.c:3170:13: error: array subscript -1 is outside array bounds of ‘char[1024]’ [-Werror=array-bounds]
   3170 |       output[0] = '\t';
        |       ~~~~~~^~~
  config.c:3076:7: note: while referencing ‘buf’
   3076 |  char buf[1024];
        |       ^~~

If we were to ever switch off of fgets() to strbuf_getline() or similar,
we'd probably need to use size_t to avoid other overflow problems. But
for now we know we're safe because of the small fixed size of our
buffer.

Signed-off-by: Jeff King <peff@peff.net>
---
 config.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Junio C Hamano Aug. 4, 2020, 4:30 p.m. UTC | #1
Jeff King <peff@peff.net> writes:

> Compiling with gcc-10, -O2, and -fsanitize=undefined results in a
> compiler warning:
>
>   config.c: In function ‘git_config_copy_or_rename_section_in_file’:
>   config.c:3170:17: error: writing 1 byte into a region of size 0 [-Werror=stringop-overflow=]
>    3170 |       output[0] = '\t';
>         |       ~~~~~~~~~~^~~~~~
>   config.c:3076:7: note: at offset -1 to object ‘buf’ with size 1024 declared here
>    3076 |  char buf[1024];
>         |       ^~~
>
> This is a false positive. The interesting lines of code are:
>
>   int i;
>   char *output = buf;
>   ...
>   for (i = 0; buf[i] && isspace(buf[i]); i++)
>           ; /* do nothing */
>   ...
>   int offset;
>   offset = section_name_match(&buf[i], old_name);
>   if (offset > 0) {
>           ...
>           output += offset + i;
>           if (strlen(output) > 0) {
> 		  /*
> 		   * More content means there's
> 		   * a declaration to put on the
> 		   * next line; indent with a
> 		   * tab
> 		   */
> 		  output -= 1;
> 		  output[0] = '\t';
> 	  }
>   }
>
> So we do assign output to buf initially. Later we increment it based on
> "offset" and "i" and then subtract "1" from it. That latter step is what
> the compiler is complaining about; it could lead to going off the left
> side of the array if "output == buf" at the moment of the subtraction.
> For that to be the case, then "offset + i" would have to be 0. But that
> can't happen:
>
>   - we know that "offset" is at least 1, since we're in a conditional
>     block that checks that
>
>   - we know that "i" is not negative, since it started at 0 and only
>     incremented over whitespace
>
> So the sum must be at least 1, and therefore it's OK to subtract one
> from "output".
>
> But that's not quite the whole story. Since "i" is an int, it could in
> theory be possible to overflow to negative (when counting whitespace on
> a very large string). But we know that's impossible because we're
> counting the 1024-byte buffer we just fed to fgets(), so it can never be
> larger than that.
>
> Switching the type of "i" to "unsigned" makes the warning go away, so
> let's do that.
>
> Arguably size_t is an even better type (for this and for the other
> length fields), but switching to it produces a similar but distinct
> warning:
>
>   config.c: In function ‘git_config_copy_or_rename_section_in_file’:
>   config.c:3170:13: error: array subscript -1 is outside array bounds of ‘char[1024]’ [-Werror=array-bounds]
>    3170 |       output[0] = '\t';
>         |       ~~~~~~^~~
>   config.c:3076:7: note: while referencing ‘buf’
>    3076 |  char buf[1024];
>         |       ^~~
>
> If we were to ever switch off of fgets() to strbuf_getline() or similar,
> we'd probably need to use size_t to avoid other overflow problems. But
> for now we know we're safe because of the small fixed size of our
> buffer.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---

Thanks.  80 lines of informative log message to explain a one liner
was surprisingly pleasnt to read.  Nicely done.

>  config.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/config.c b/config.c
> index 8db9c77098..2b79fe76ad 100644
> --- a/config.c
> +++ b/config.c
> @@ -3115,7 +3115,7 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename
>  	}
>  
>  	while (fgets(buf, sizeof(buf), config_file)) {
> -		int i;
> +		unsigned i;
>  		int length;
>  		int is_section = 0;
>  		char *output = buf;
Taylor Blau Aug. 5, 2020, 3:15 p.m. UTC | #2
On Tue, Aug 04, 2020 at 09:30:15AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > Compiling with gcc-10, -O2, and -fsanitize=undefined results in a
> > compiler warning:
> >
> >   config.c: In function ‘git_config_copy_or_rename_section_in_file’:
> >   config.c:3170:17: error: writing 1 byte into a region of size 0 [-Werror=stringop-overflow=]
> >    3170 |       output[0] = '\t';
> >         |       ~~~~~~~~~~^~~~~~
> >   config.c:3076:7: note: at offset -1 to object ‘buf’ with size 1024 declared here
> >    3076 |  char buf[1024];
> >         |       ^~~
> >
> > This is a false positive. The interesting lines of code are:
> >
> >   int i;
> >   char *output = buf;
> >   ...
> >   for (i = 0; buf[i] && isspace(buf[i]); i++)
> >           ; /* do nothing */
> >   ...
> >   int offset;
> >   offset = section_name_match(&buf[i], old_name);
> >   if (offset > 0) {
> >           ...
> >           output += offset + i;
> >           if (strlen(output) > 0) {
> > 		  /*
> > 		   * More content means there's
> > 		   * a declaration to put on the
> > 		   * next line; indent with a
> > 		   * tab
> > 		   */
> > 		  output -= 1;
> > 		  output[0] = '\t';
> > 	  }
> >   }
> >
> > So we do assign output to buf initially. Later we increment it based on
> > "offset" and "i" and then subtract "1" from it. That latter step is what
> > the compiler is complaining about; it could lead to going off the left
> > side of the array if "output == buf" at the moment of the subtraction.
> > For that to be the case, then "offset + i" would have to be 0. But that
> > can't happen:
> >
> >   - we know that "offset" is at least 1, since we're in a conditional
> >     block that checks that
> >
> >   - we know that "i" is not negative, since it started at 0 and only
> >     incremented over whitespace
> >
> > So the sum must be at least 1, and therefore it's OK to subtract one
> > from "output".
> >
> > But that's not quite the whole story. Since "i" is an int, it could in
> > theory be possible to overflow to negative (when counting whitespace on
> > a very large string). But we know that's impossible because we're
> > counting the 1024-byte buffer we just fed to fgets(), so it can never be
> > larger than that.
> >
> > Switching the type of "i" to "unsigned" makes the warning go away, so
> > let's do that.
> >
> > Arguably size_t is an even better type (for this and for the other
> > length fields), but switching to it produces a similar but distinct
> > warning:
> >
> >   config.c: In function ‘git_config_copy_or_rename_section_in_file’:
> >   config.c:3170:13: error: array subscript -1 is outside array bounds of ‘char[1024]’ [-Werror=array-bounds]
> >    3170 |       output[0] = '\t';
> >         |       ~~~~~~^~~
> >   config.c:3076:7: note: while referencing ‘buf’
> >    3076 |  char buf[1024];
> >         |       ^~~
> >
> > If we were to ever switch off of fgets() to strbuf_getline() or similar,
> > we'd probably need to use size_t to avoid other overflow problems. But
> > for now we know we're safe because of the small fixed size of our
> > buffer.
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
>
> Thanks.  80 lines of informative log message to explain a one liner
> was surprisingly pleasnt to read.  Nicely done.

Agreed, and sorry that this took me so long to read (I thought that I
had read it when you sent it, but apparently not). Your reasoning is
sensible, and I agree that your fix is appropriate.

  Reviewed-by: Taylor Blau <me@ttaylorr.com>

> >  config.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/config.c b/config.c
> > index 8db9c77098..2b79fe76ad 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -3115,7 +3115,7 @@ static int git_config_copy_or_rename_section_in_file(const char *config_filename
> >  	}
> >
> >  	while (fgets(buf, sizeof(buf), config_file)) {
> > -		int i;
> > +		unsigned i;
> >  		int length;
> >  		int is_section = 0;
> >  		char *output = buf;

Thanks,
Taylor

Patch
diff mbox series

diff --git a/config.c b/config.c
index 8db9c77098..2b79fe76ad 100644
--- a/config.c
+++ b/config.c
@@ -3115,7 +3115,7 @@  static int git_config_copy_or_rename_section_in_file(const char *config_filename
 	}
 
 	while (fgets(buf, sizeof(buf), config_file)) {
-		int i;
+		unsigned i;
 		int length;
 		int is_section = 0;
 		char *output = buf;