Fix the remaining heap-buffer-overflow (read) error in arcfour.c.

nss/lib/freebl/arcfour.c

Index: nss/lib/freebl/arcfour.c
===================================================================
--- nss/lib/freebl/arcfour.c	(revision 197511)
+++ nss/lib/freebl/arcfour.c	(working copy)
@@ -372,7 +372,6 @@
 	register Stype tmpSi, tmpSj;
 	register PRUint8 tmpi = cx->i;
 	register PRUint8 tmpj = cx->j;
-	unsigned int byteCount;

[wtc, 2013/05/07 18:00:50]

I moved this variable declaration to the lexical scope where
this variable is used.

 	unsigned int bufShift, invBufShift;
 	unsigned int i;
 	const unsigned char *finalIn;
@@ -390,7 +389,7 @@
 	*outputLen = inputLen;
 	pInWord = (const WORD *)(input - inOffset);
 	pOutWord = (WORD *)(output - outOffset);
-	if (inOffset < outOffset) {
+	if (inOffset <= outOffset) {

[wtc, 2013/05/07 18:00:50]

How to review this change.

1. Verify that bufShift and invBufShift are not used if
inOffset == outOffset. So, the values of bufShift and
invBufShift don't really matter in that case.

2. If inOffset == outOffset, one of bufShift and invBufShift
is 0 and the other is 8*WORDSIZE.

3. In the original code, invBufShift is 0 if inOffset ==
outOffset. In the new code, bufShift is 0 in that case
because it makes the code below look a little nicer.

 		bufShift = 8*(outOffset - inOffset);
 		invBufShift = 8*WORDSIZE - bufShift;
 	} else {
@@ -406,7 +405,7 @@
 	/* least one partial word of input should ALWAYS be loaded.      */
 	/*****************************************************************/
 	if (outOffset) {
-		byteCount = WORDSIZE - outOffset; 
+		unsigned int byteCount = WORDSIZE - outOffset; 
 		for (i = 0; i < byteCount; i++) {
 			ARCFOUR_NEXT_BYTE();
 			output[i] = cx->S[t] ^ input[i];
@@ -466,10 +465,6 @@
 			inWord = 0;
 		}
 	}
-	/* Output buffer is aligned, inOffset is now measured relative to
-	 * outOffset (and not a word boundary).
-	 */
-	inOffset = (inOffset + WORDSIZE - outOffset) % WORDSIZE;

[wtc, 2013/05/07 18:00:50]

This redefinition of inOffset is very confusing. It is an
attempt to reuse a local variable. An optimizing compiler
can do this for us.

I moved one local variable (byteCount) into a smaller
lexical scope to help the compiler reuse it for something
else, such as the new preloadedByteCount variable.

 	/*****************************************************************/
 	/* Step 2: main loop                                             */
 	/* At this point the output buffer is word-aligned.  Any unused  */
@@ -477,8 +472,13 @@
 	/* the input buffer is unaligned relative to the output buffer,  */
 	/* shifting has to be done.                                      */
 	/*****************************************************************/
-	if (inOffset) {
-		for (; inputLen >= WORDSIZE; inputLen -= WORDSIZE) {
+	if (bufShift) {

[wtc, 2013/05/07 18:00:50]

With the new values of bufShift and invBufShift above, if
bufShift is nonzero, it means inOffset != outOffset, which
is the condition described in the comment above.

+		/* preloadedByteCount is the number of input bytes pre-loaded
+		 * in inWord.
+		 */
+		unsigned int preloadedByteCount = bufShift/8;
+		for (; inputLen >= preloadedByteCount + WORDSIZE;
+		     inputLen -= WORDSIZE) {

[wtc, 2013/05/07 18:00:50]

How to understand this change:

1. At the beginning of each loop iteration, inWord already
contains some input bytes. Let preloadedByteCount be that
value.

We are about to read WORDSIZE bytes using *pInWord. So
we need to make sure we still have preloadedByteCount + WORDSIZE
bytes of input available to avoid reading beyond the end
of the buffer.

This is why the loop condition should be
    inputLen >= preloadedByteCount + WORDSIZE
rather than just
    inputLen >= WORDSIZE

2. Now, what is the value of preloadedByteCount? An easy
way to see that is line 483:

    inWord |= nextInWord RSH bufShift;

We need to shift nextInWord by bufShift bits to avoid
overwriting the preloaded input bytes in inWord. This
means preloadedByteCount is bufShift/8 bytes.

3. Finally, on line 495 we can compute the finalIn pointer
using the new preloadedByteCount variable now. To get the
remaining input, we need to go backward from pInWord by
preloadedByteCount bytes.

 			nextInWord = *pInWord++;
 			inWord |= nextInWord RSH bufShift;
 			nextInWord = nextInWord LSH invBufShift;
@@ -492,7 +492,7 @@
 			cx->j = tmpj;
 			return SECSuccess;
 		}
-		finalIn = (const unsigned char *)pInWord - WORDSIZE + inOffset;
+		finalIn = (const unsigned char *)pInWord - preloadedByteCount;
 	} else {
 		for (; inputLen >= WORDSIZE; inputLen -= WORDSIZE) {
 			inWord = *pInWord++;