MantisBT - Community
View Issue Details
0022970Community[OCCT] OCCT:Visualizationpublic2012-02-02 03:412014-01-22 12:52
pdolbey 
aba 
normalminor 
closedfixed 
ALL
[OCCT] 6.5.2 
[OCCT] 6.6.0[OCCT] 6.6.0 
Not needed
0022970: Incorrect array use in Graphic3d_StructureManager.cxx
In Graphic3d_StructureManager we see the code

/* Initialize PHIGS and start up */
        if (Initialisation) {

                Initialisation = Standard_False;
                /* table to manage IDs of StructureManager */
                for (i=1; i<=Limit; i++) StructureManager_ArrayId[i] = 0;

                StructureManager_CurrentId = 1;
                StructureManager_ArrayId[1] = 1;

        }
        else {
                for (i=1; i<=Limit && NotFound; i++)
                        if (StructureManager_ArrayId[i] == 0) {
                                NotFound = Standard_False;
                                StructureManager_CurrentId = i;
                                StructureManager_ArrayId[i] = 1;
                        }

                if (NotFound)
                        Graphic3d_InitialisationError::Raise
                                        ("Too many ViewManagers are defined");
        }

The for loops run from 1 to <=Limit (which is 100), but this is not FORTRAN :). In c++ arrays are accessed from 0 to <Limit - this error runs the risk of overwriting memory. How this incorrect use of array indexing propagates into the rest of the code needs to be checked.

Pete
None
No tags attached.
child of 0022832closed aba Community Not documented limitation of Graphic3d_StructureManager 
Issue History
2012-02-02 03:41pdolbeyNew Issue
2012-02-02 03:41pdolbeyAssigned To => san
2012-02-02 07:48abvRelationship addedchild of 0022832
2012-02-02 08:02abvNote Added: 0019339
2012-10-20 08:14abvTarget Version => 6.6.0
2012-10-24 11:19sanStatusnew => assigned
2012-10-24 11:19sanAssigned Tosan => aba
2012-10-24 20:46sanNote Added: 0021933
2012-10-25 12:29abaNote Added: 0021939
2012-10-25 14:15abaNote Added: 0021943
2012-10-25 14:16abaNote Added: 0021944
2012-10-25 14:16abaStatusassigned => resolved
2012-10-25 14:16abaAssigned Toaba => san
2012-10-26 23:53sanNote Added: 0021979
2012-10-26 23:53sanAssigned Tosan => bugmaster
2012-10-26 23:53sanStatusresolved => reviewed
2012-11-02 15:00apnNote Added: 0022079
2012-11-02 15:01apnTest case number => Not needed
2012-11-02 15:01apnStatusreviewed => tested
2012-11-16 13:03abaChangeset attached => occt master f163f612
2012-11-16 13:03abaAssigned Tobugmaster => aba
2012-11-16 13:03abaStatustested => verified
2012-11-16 13:03abaResolutionopen => fixed
2012-12-10 17:16abaChangeset attached => occt master f163f612
2013-04-23 13:36aivStatusverified => closed
2013-04-29 15:21aivFixed in Version => 6.6.0
2014-01-22 12:52abvRelationship addedhas duplicate 0024551

Notes
(0019339)
abv   
2012-02-02 08:02   
We actually have a potential out-of-bound problem here, since function Limit() returns StructureManager_MAX, which is the size of the StructureManager_ArrayId array. Hence the last step of the cycle will go behind the array limits.

Even if the out-of-bound problem can be fixed easily, having static array of fixed size is nonsense nowadays. Please consider if it is needed at all.
(0021933)
san   
2012-10-24 20:46   
It might be helpful if the exception message is changed to something clearer, e.g. "You are trying to create too many ViewManagers at the same time! The number of simultaneously created ViewManagers cannot exceed NNN" - where NNN should be replaced by Limit value.

If IDs are re-used by view managers if one is destroyed and another one is then created (this is to be checked), the current logic can be kept. Naturally, array indexing mistake (index starts from 0 and ends at Limit - 1) should be corrected.
(0021939)
aba   
2012-10-25 12:29   
It was checked and confirmed that after destruction of one view manager its ID will be re-used.
(0021943)
aba   
2012-10-25 14:15   
Bounds of loops in lines 96 and 103 were corrected according to bounds of the array StructureManager_ArrayId.
Initialisation error message was changed to more informative one.
(0021944)
aba   
2012-10-25 14:16   
The Git branch CR22970 is ready to be reviewed.
Please review.
(0021979)
san   
2012-10-26 23:53   
Branch CR22970 reviewed without remarks, ready for testing.
(0022079)
apn   
2012-11-02 15:00   
Dear BugMaster,
Branch CR22970 (and products from GIT master) was compiled on Linux and Windows platforms and tested.

Regression:
Not detected

Improvements:
Not detected

Testing case:
Not needed