[Openais] [PATCH 1/8] COVERITY 12: prevent overrun of output buffers.

[angus salkeld]

On Fri, 2009-11-20 at 18:08 -0700, Steven Dake wrote:
> On Sun, 2009-10-25 at 11:56 +1300, Angus Salkeld wrote:
> > Overrun of static array "normal_output_buffer" of size 2048
> > at position 2048 with index variable "normal_output_buffer_idx"
> > 
> > 536  		normal_output_buffer[normal_output_buffer_idx] = '\0';
> > 537  		syslog_output_buffer[syslog_output_buffer_idx] = '\0';
> > 538
> > 
> > Signed-off-by: Angus Salkeld <angus.salkeld at gmail.com>
> > ---
> >   exec/logsys.c |    4 ++--
> >   1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/exec/logsys.c b/exec/logsys.c
> > index 978c157..395a59d 100644
> > --- a/exec/logsys.c
> > +++ b/exec/logsys.c
> > @@ -527,8 +527,8 @@ static void log_printf_to_logs (
> >   			syslog_output_buffer_idx += syslog_len;
> >   			format_buffer_idx += 1;
> >   		}
> > -		if ((normal_output_buffer_idx == sizeof (normal_output_buffer)) ||
> > -		    (syslog_output_buffer_idx == sizeof (syslog_output_buffer))) {
> > +		if ((normal_output_buffer_idx >= sizeof (normal_output_buffer - 2)) ||
> > +		    (syslog_output_buffer_idx >= sizeof (syslog_output_buffer - 1))) {
> >   			break;
> >   		}
> >   	}
> 
> this code doesn't make any sense.  the code takes the sizeof a buffer-2
> (which if i parse with my eyes is sizeof (&buffer[-2])).  instead it
> should be something like sizeof (normal_output_buffer) - 1.
> 
> This raises a further question why use - 2 in one stanza and -1 in
> another?  Wouldn't they both be -1?
> 
> Regards
> -steve
> 
> 

Well spotted, I "just" added the " - 1" in the wrong place - oops.

Here is a fixed patch (with a comment).

-Angus

===================================================================
--- exec/logsys.c	(revision 2546)
+++ exec/logsys.c	(working copy)
@@ -527,8 +527,12 @@
 			syslog_output_buffer_idx += syslog_len;
 			format_buffer_idx += 1;
 		}
-		if ((normal_output_buffer_idx == sizeof (normal_output_buffer)) ||
-		    (syslog_output_buffer_idx == sizeof (syslog_output_buffer))) {
+		if ((normal_output_buffer_idx >= sizeof (normal_output_buffer) - 2) ||
+		    (syslog_output_buffer_idx >= sizeof (syslog_output_buffer) - 1)) {
+			/* Note: we make allowance for '\0' at the end of
+			 * both of these arrays and normal_output_buffer also
+			 * needs a '\n'.
+			 */
 			break;
 		}
 	}