Closed
Bug 1248551
Opened 9 years ago
Closed 9 years ago
[Static Analysis][Improper use of negative value][Uninitialized scalar variable] In function nsBidi::ResolveImplicitLevels
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: andi, Assigned: andi)
References
(Blocks 1 open bug)
Details
(Keywords: coverity, Whiteboard: CID 1296735, CID 1346244)
Attachments
(1 file, 1 obsolete file)
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;
>> }
Assignee | ||
Comment 1•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/35083/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35083/
Attachment #8719734 -
Flags: review?(roc)
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
Attachment #8719734 -
Flags: review?(roc)
Assignee | ||
Updated•9 years ago
|
Attachment #8719734 -
Flags: review?(jfkthame)
Comment 3•9 years ago
|
||
(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•9 years ago
|
||
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.
Attachment #8719734 -
Flags: review?(jfkthame) → review-
Comment 5•9 years ago
|
||
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...
Attachment #8720335 -
Flags: review?(roc)
Assignee | ||
Comment 6•9 years ago
|
||
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 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.
Attachment #8720335 -
Flags: review?(roc) → review+
Comment 8•9 years ago
|
||
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
Assignee | ||
Updated•9 years ago
|
Attachment #8719734 -
Attachment is obsolete: true
Comment 9•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in
before you can comment on or make changes to this bug.
Description
•