Bug 149891 - 2 * array index out of range
2 * array index out of range
Status: CLOSED INSUFFICIENT_DATA
Product: Fedora
Classification: Fedora
Component: ninja (Show other bugs)
3
All Linux
medium Severity medium
: ---
: ---
Assigned To: Adrian Reber
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2005-02-28 12:29 EST by David Binderman
Modified: 2008-02-06 19:18 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-02-06 19:18:43 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description David Binderman 2005-02-28 12:29:14 EST
Description of problem:

I just tried to compile package ninja-1.5.8.1-2 from 
Redhat Fedora Extras development tree.

The compiler said

1.

/usr/src/redhat/BUILD/ninja-1.5.8.1/source/ninjacmd.c(1377): warning
#175: subscript out of range

The source code is

             *(deadppl[lines] + DEADPPL_SIZE - 1) = '\0';

but

   u_char *chann, deadppl[DEADPPL_SIZE][MAX_DEADPPL],

and

#define DEADPPL_SIZE 1024
#define MAX_DEADPPL 200

so the program is trying to index 1023 characters into an array of
size 200.

Suggest rework.

2.

/usr/src/redhat/BUILD/ninja-1.5.8.1/source/ninjacmd.c(1387): warning
#175: subscript out of range

Duplicate.


Version-Release number of selected component (if applicable):


How reproducible:


Steps to Reproduce:
1.
2.
3.
  
Actual results:


Expected results:


Additional info:
Comment 1 Adrian Reber 2005-02-28 13:45:38 EST
You just like doing this... don't you :-)

Now that I know you are doing this with the intel compiler I know
where it comes from but you should definitely specify this in your bug
reports.

I will have a look at it.
Comment 2 Adrian Reber 2005-03-11 02:14:52 EST
so what about this patch:

--- ninja-1.5.8.1/source/ninjacmd.c     2002-02-17 11:24:39.000000000
+0100
+++ ninja-1.5.8.1.new/source/ninjacmd.c 2005-03-11 08:13:39.000000000
+0100
@@ -1305,7 +1305,7 @@
    u_char *command, *args, *subargs;
 {
 #define DEADPPL_SIZE 1024
-#define MAX_DEADPPL 200
+#define MAX_DEADPPL 1024
 #define MAX_SAFEPPL 20
 #define NINJA_BAN_MASK "ninja!irc@rulez"
    u_char *chann, deadppl[DEADPPL_SIZE][MAX_DEADPPL],
*safeppl[MAX_SAFEPPL];

can you try this?
Comment 3 David Binderman 2005-03-11 04:48:58 EST
>so what about this patch:

Good comedy. I strongly recommend avoiding this patch.

Suggest change the mention of DEADPPL_SIZE to MAX_DEADPPL in array
index. Suggest don't change the value of any macros.
Comment 4 Adrian Reber 2005-03-11 04:54:13 EST
so do you like this better:

--- ninja-1.5.8.1/source/ninjacmd.c     2002-02-17 11:24:39.000000000
+0100
+++ ninja-1.5.8.1.new/source/ninjacmd.c 2005-03-11 10:53:44.000000000
+0100
@@ -1308,7 +1308,7 @@
 #define MAX_DEADPPL 200
 #define MAX_SAFEPPL 20
 #define NINJA_BAN_MASK "ninja!irc@rulez"
-   u_char *chann, deadppl[DEADPPL_SIZE][MAX_DEADPPL],
*safeppl[MAX_SAFEPPL];
+   u_char *chann, deadppl[DEADPPL_SIZE][DEADPPL_SIZE],
*safeppl[MAX_SAFEPPL];
    u_char tmpbuf1[1024];
    Channel *chanl;
    Nick *nicks;
Comment 5 David Binderman 2005-03-11 14:59:48 EST
>so do you like this better:

No I most emphatically don't. 

It is, IMHO, as bad as your previous proposed solution.

I am struggling to come up with better advice than "Please read my
previous postings on this subject".
Comment 6 Adrian Reber 2005-03-14 01:51:31 EST
Then please attach a patch which fixes it, because I don't know how to
fix it.
Comment 7 David Binderman 2005-03-14 05:13:02 EST
>I don't know how to fix it.

Surprising. I thought my description was sufficient.

More directly:

Old code

      *(deadppl[lines] + DEADPPL_SIZE - 1) = '\0';

suggested new code

      *(deadppl[lines] + MAX_DEADPPL - 1) = '\0';

This is untested.
Comment 8 Michael Schwendt 2005-03-14 05:57:40 EST
In the given context that looks wrong. It would only shut up the
compiler, but not fix the broken code:

deadppl is a two-dimensional array of u_char, dimensions being
DEADPPL_SIZE * MAX_DEADPPL = 1024 * 200

The following line of code would exceed array boundaries, since second
argument DEADPPL_SIZE-1 is 1023 and much larger than second array
dimension MAX_DEADPPL=200.

    snprintf(deadppl[lines], DEADPPL_SIZE - 1, "MODE %s -b%s %s %s",
chanl->channel, strfill('o', count), NINJA_BAN_MASK, tmpbuf1);

In the context of the code you can observe that deadppl is supposed to
be Y lines of text, each being X characters long, X=DEADPPL_SIZE,
Y=MAX_DEADPPL. Early in the code, MAX_DEADPPL lines (the second
dimension) are cleared with a leading zero:
 
   for (i = 0; i < MAX_DEADPPL; i++)
     *deadppl[i] = 0;

Hence my fix proposal would be to reverse the array dimensions and
make the entire code more correct:

--- ninjacmd.c.orig     2005-03-14 11:38:18.000000000 +0100
+++ ninjacmd.c  2005-03-14 11:46:54.000000000 +0100
@@ -1308,7 +1308,7 @@
 #define MAX_DEADPPL 200
 #define MAX_SAFEPPL 20
 #define NINJA_BAN_MASK "ninja!irc@rulez"
-   u_char *chann, deadppl[DEADPPL_SIZE][MAX_DEADPPL],
*safeppl[MAX_SAFEPPL];
+   u_char *chann, deadppl[MAX_DEADPPL][DEADPPL_SIZE],
*safeppl[MAX_SAFEPPL];
    u_char tmpbuf1[1024];
    Channel *chanl;
    Nick *nicks;


Note, however, that the existing code in the ninjacmd.c file does not
check whether more than MAX_DEADPPL lines are ever created in the
buffer. MAX_DEADPPL is only used to create the array. The later loop
over the list of nicks does not contain a safety check.
Comment 9 David Binderman 2005-03-14 09:33:51 EST
>In the given context that looks wrong. 

It may look wrong to you, but it looks very correct to me.

>It would only shut up the compiler, but not fix the broken code:

As I see it, it would not only shut up the compiler, but also fix
the broken code.

>Hence my fix proposal would be to reverse the array dimensions and
>make the entire code more correct:

Please don't reverse the array dimensions. 
You run the risk of introducing a whole set of new bugs.

I can't really help you much further with this bug report.

I've had multiple goes at trying to explain what I beleive
is the correct solution without success.

I suggest you show this bug report to a more experienced programmer
friend or colleague at your site for help. 

Maybe they can explain the problem and my proposed solution to you
better than I can.
Comment 10 Michael Schwendt 2005-03-14 10:09:46 EST
Your fix is just plain wrong. Re-read my previous comment. Pay extra
attention to the snprintf line. This is how it would look with your
suggestion applied:

snprintf(deadppl[lines], DEADPPL_SIZE - 1, "MODE %s -b%s %s %s",
    chanl->channel, strfill('o', count), NINJA_BAN_MASK, tmpbuf1);
*(deadppl[lines] + MAX_DEADPPL - 1) = '\0';

Do you realise where you insert the zero? Do you see that the snprintf
can exceed the array's boundary?
Comment 11 Michael Schwendt 2005-03-14 10:11:19 EST
Oh, btw, I joined here in comment 8. You talked to Adrian before, not me.
Comment 12 David Binderman 2005-03-14 10:38:56 EST
>Your fix is just plain wrong. 

The original declaration of the array is 

u_char *chann, deadppl[DEADPPL_SIZE][MAX_DEADPPL]

which means that the array is 1024 * 200 chars.

Like I said originally, the source code the compiler complains
about is

 *(deadppl[lines] + DEADPPL_SIZE - 1) = '\0';

For the record, this is the same as

 deadppl[lines][ DEADPPL_SIZE - 1] = '\0';

which, like I said originally, is indexing, on the 2nd dimension,
1023 chars into an array of size 200.

Which is what the compiler and I am are complaining about.

I don't know where you get the line with the snprintf from.

I can guess that either it is the duplicate that I mentioned
in the original post, or the source code has moved on since
my bug report, and you are trying to fix some nearby code.

In either case, I can't help you fix the snprintf you seem 
so keen to tell me about, but seems unrelated to my original
bug report. 
Comment 13 Michael Schwendt 2005-03-14 11:15:07 EST
No, no, no. The source file has not changed since February 2002. I
recommend you read the rest of the code where the deadppl array is
accessed before trying to fix something. As I wrote earlier, you only
try to shut up the compiler in a specific line, not fix the code.
Please return to comment 8. Thank you.

Btw, I've sent my patch upstream to Joshua Drake, not knowing whether
Adrian has forwarded the bug report already before.
Comment 14 David Binderman 2005-03-14 11:42:16 EST
>I recommend you read the rest of the code

Sorry, but that disk has now been reformatted and
installed with something else.

>you only try to shut up the compiler in a specific line, not fix the
>code.

Agreed. The bug report is about broken array indexing. As
far as I am concerned, that is the sum total of the bug report.

If you want to go further, then that is up to you, and AFAIK
outside the original scope of this bug report.
Comment 15 Adrian Reber 2005-03-17 13:44:21 EST
> Btw, I've sent my patch upstream to Joshua Drake, not knowing whether
> Adrian has forwarded the bug report already before.

No I haven't contacted upstream yet. So thanks for doing this.
Comment 16 Christian Iseli 2007-01-17 18:21:41 EST
FC3 and FC4 have now been EOL'd.

Please check the ticket against a current Fedora release, and either adjust the
release number, or close it if appropriate.

Thanks.

Your friendly BZ janitor :-)
Comment 17 petrosyan 2008-02-06 19:18:43 EST
Fedora Core 3 is not maintained anymore.

Setting status to "INSUFFICIENT_DATA". If you can reproduce this bug in the
current Fedora release please reopen this bug and assign it to the corresponding
Fedora version.

Note You need to log in before you can comment on or make changes to this bug.