1248551 - [Static Analysis][Improper use of negative value][Uninitialized scalar variable] In function nsBidi::ResolveImplicitLevels

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Andi [:andi]]

The Static Analysis tool Coverity signaled two issues regarding variable levState.startON

1.Uninitialized scalar variable  - variable levState.startON can be used in function ProcessPropertySeq without being initialized:

>>  start0 = start;                         /* save original start position */
>>  oldStateSeq = (uint8_t)pLevState->state;
>>  cell = pImpTab[oldStateSeq][_prop];
>>  pLevState->state = GET_STATE(cell);       /* isolate the new state */
>>  actionSeq = pImpAct[GET_ACTION(cell)]; /* isolate the action */
>>  addLevel = pImpTab[pLevState->state][IMPTABLEVELS_RES];
>>
>>  if(actionSeq) {
>>    switch(actionSeq) {
>>    case 1:                         /* init ON seq */
>>      pLevState->startON = start0;
>>      break;
>>
>>    case 2:                         /* prepend ON seq to current seq */
>>      start = pLevState->startON;
>>      break;
>>
>>    default:                        /* we should never get here */
>>      MOZ_ASSERT(false);
>>      break;
>>    }
>>  } 

This function is called in several places from ResolveImplicitLevels:

a. 
>>    } else {
>>      stateImp = 0;
>>    }
>>    levState.state = 0;
>>    ProcessPropertySeq(&levState, aSOR, aStart, aStart);
>>  }

b. 

>>      case 1:             /* process current seq1, init new seq1 */
>>        ProcessPropertySeq(&levState, resProp, start1, i);
>>        start1 = i;
>>        break;
>>      case 2:             /* init new seq2 */
>>        start2 = i;
>>        break;
>>      case 3:             /* process seq1, process seq2, init new seq1 */
>>        ProcessPropertySeq(&levState, resProp, start1, start2);
>>        ProcessPropertySeq(&levState, DirProp_ON, start2, i);
>>        start1 = i;
>>        break;
>>      case 4:             /* process seq1, set seq1=seq2, init new seq2 */
>>        ProcessPropertySeq(&levState, resProp, start1, start2);
>>        start1 = start2;
>>        start2 = i;
>>        break;
>>      default:            /* we should never get here */
>>        MOZ_ASSERT(false);
>>        break;
>>      }

c. 

>>    mIsolates[mIsolateCount].start1 = start1;
>>  } else {
>>    ProcessPropertySeq(&levState, aEOR, aLimit, aLimit);
>>  }

I can't say for sure that the execurion of ProcessPropertySeq  will fall on case 1: thus setting the variable and i guess it wouldn't hurt in memset the entire structure after it's declaration.

2.Improper use of negative value - if execution will fall on this path:

>>else {
>>    levState.startON = -1;
>>    start1 = aStart;
>>    if (dirProps[aStart] == NSM) {
>>      stateImp = 1 + aSOR;
>>    } else {
>>      stateImp = 0;
>>    }
>>    levState.state = 0;
>>    ProcessPropertySeq(&levState, aSOR, aStart, aStart);

levState.startON is set to -1 leading to a posible bad access in function ProcessPropertySeq  where:

>>    case 2:                         /* prepend ON seq to current seq */
>>      start = pLevState->startON;
>>      break;
.....
>>  if(addLevel || (start < start0)) {
>>    level = pLevState->runLevel + addLevel;
>>    if (start >= pLevState->runStart) {
>>      for (k = start; k < limit; k++) {
>>        levels[k] = level;
>>      }

Comment 1

[Andi [:andi]]

Attachment: MozReview Request: Bug 1248551 - memset levState at the begining of ResolveImplicitLevels and avoid assigning -1 to levState.startON that could cause a bad access. r?roc

Review commit: https://reviewboard.mozilla.org/r/35083/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35083/

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8719734 [details]
MozReview Request: Bug 1248551 - memset levState at the begining of ResolveImplicitLevels and avoid assigning -1 to levState.startON that could cause a bad access. r?roc

https://reviewboard.mozilla.org/r/35083/#review31751

Please reassign the review to jfkthame

Comment 3

[Jonathan Kew [:jfkthame]]

(In reply to Andi-Bogdan Postelnicu from comment #0)

AFAICT, I don't think issue 2 is real:

> 2.Improper use of negative value - if execution will fall on this path:
> 
> >>else {
> >>    levState.startON = -1;
> >>    start1 = aStart;
> >>    if (dirProps[aStart] == NSM) {
> >>      stateImp = 1 + aSOR;
> >>    } else {
> >>      stateImp = 0;
> >>    }
> >>    levState.state = 0;
> >>    ProcessPropertySeq(&levState, aSOR, aStart, aStart);
> 
> levState.startON is set to -1 leading to a posible bad access in function
> ProcessPropertySeq  where:
> 
> >>    case 2:                         /* prepend ON seq to current seq */
> >>      start = pLevState->startON;
> >>      break;

This case should never occur when pLevState->startON is still -1, because action 2 only occurs in states where we have seen characters of category ON, and so pLevState->startON will have been set.

Note that in impTabL, action 2 occurs only in states 4 and 5; and those states can only be reached via transitions on ON or S that execute action 1. And in impTabR, action 2 occurs only in state 4, which is likewise reachable only via transitions that execute action 1.

(We can add an assertion here to confirm that pLevState->startON is non-negative.)

> .....
> >>  if(addLevel || (start < start0)) {
> >>    level = pLevState->runLevel + addLevel;
> >>    if (start >= pLevState->runStart) {
> >>      for (k = start; k < limit; k++) {
> >>        levels[k] = level;
> >>      }

Because case 2 (above) is never reached with pLevState->startON == -1, we won't have start == -1 here and so there's no risk of an illegal negative index on levels[]. (Actually, even if we did have negative start here, the code would be safe because the array access is conditional on start >= pLevState->runStart, and runStart will be >= 0.)

And for the same reason as above, that the transitions in the state tables impTabL and impTabR guarantee that action 1 will always occur before action 2, we never try to read the value of startON unless it has already been set on an earlier character. So issue 1 is also a false positive, I believe.

So I think this is a case where Coverity's analysis falls short, and the code is in fact OK as written. As such, I'd be reluctant to add the overhead of explicitly clearing the struct; and initializing levState.startON to -1 rather than 0 in ResolveImplicitLevels() is preferable because it is clear that this cannot be a valid start position.

Comment 4

[Jonathan Kew [:jfkthame]]

Comment on attachment 8719734 [details]
MozReview Request: Bug 1248551 - memset levState at the begining of ResolveImplicitLevels and avoid assigning -1 to levState.startON that could cause a bad access. r?roc

As described in comment 3, I don't think we should do this.

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: Add assertion to confirm that the bidi code is not trying to execute an invalid state-machine action

What I think we could usefully do here is to add an assertion. (Not sure if this will help coverity at all.) If Jesse's fuzzers manage to hit it, then we'll know something is amiss...

Comment 6

[Andi [:andi]]

hello Jonathan this not being a bug i can easily mark it in Coverity as false positive and wave the checker so i don't think we need an assertion.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8720335 [details] [diff] [review]
Add assertion to confirm that the bidi code is not trying to execute an invalid state-machine action

Review of attachment 8720335 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsBidi.cpp
@@ +1491,5 @@
>    levState.pImpTab = impTab[levState.runLevel & 1];
>    levState.pImpAct = impAct0;
> +#ifdef DEBUG
> +  levState.startON = -1; /* initialize to invalid start position */
> +#endif

No point in making this #ifdef DEBUG. It's practically free.

Comment 8

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/28b6f5924ce2296a626a573a06cb4be4cfd125cd
Bug 1248551 - Add assertion to confirm that the bidi code is not trying to execute an invalid state-machine action. r=roc

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/28b6f5924ce2