View Issue Details

Related ChangesetsJump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0022970CommunityOCCT:Visualizationpublic2012-02-02 03:412014-01-22 12:52
Reporterpdolbey Assigned Toaba 
PrioritynormalSeverityminor 
Status closedResolutionfixed 
PlatformAOSL 
Product Version6.5.2 
Target Version6.6.0Fixed in Version6.6.0 
Summary0022970: Incorrect array use in Graphic3d_StructureManager.cxx
DescriptionIn 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
Steps To ReproduceNone
TagsNo tags attached.
Test case numberNot needed

Relationships

child of 0022832 closedaba Community Not documented limitation of Graphic3d_StructureManager 

Activities

abv

2012-02-02 08:02

manager   ~0019339

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.

san

2012-10-24 20:46

developer   ~0021933

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.

aba

2012-10-25 12:29

developer   ~0021939

It was checked and confirmed that after destruction of one view manager its ID will be re-used.

aba

2012-10-25 14:15

developer   ~0021943

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.

aba

2012-10-25 14:16

developer   ~0021944

The Git branch CR22970 is ready to be reviewed.
Please review.

san

2012-10-26 23:53

developer   ~0021979

Branch CR22970 reviewed without remarks, ready for testing.

apn

2012-11-02 15:00

administrator   ~0022079

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

Related Changesets

occt: master f163f612

2012-10-25 10:07:11

aba

Details Diff		 0022970: Incorrect array use in Graphic3d_StructureManager.cxx

Corrected bounds of loops and initialisation error message.		 Affected Issues
0022970
mod - src/Graphic3d/Graphic3d_StructureManager.cxx Diff File

Issue History

Date Modified Username Field Change
2012-02-02 03:41 pdolbey New Issue
2012-02-02 03:41 pdolbey Assigned To => san
2012-02-02 07:48 abv Relationship added child of 0022832
2012-02-02 08:02 abv Note Added: 0019339
2012-10-20 08:14 abv Target Version => 6.6.0
2012-10-24 11:19 san Status new => assigned
2012-10-24 11:19 san Assigned To san => aba
2012-10-24 20:46 san Note Added: 0021933
2012-10-25 12:29 aba Note Added: 0021939
2012-10-25 14:15 aba Note Added: 0021943
2012-10-25 14:16 aba Note Added: 0021944
2012-10-25 14:16 aba Status assigned => resolved
2012-10-25 14:16 aba Assigned To aba => san
2012-10-26 23:53 san Note Added: 0021979
2012-10-26 23:53 san Assigned To san => bugmaster
2012-10-26 23:53 san Status resolved => reviewed
2012-11-02 15:00 apn Note Added: 0022079
2012-11-02 15:01 apn Test case number => Not needed
2012-11-02 15:01 apn Status reviewed => tested
2012-11-16 13:03 aba Changeset attached => occt master f163f612
2012-11-16 13:03 aba Assigned To bugmaster => aba
2012-11-16 13:03 aba Status tested => verified
2012-11-16 13:03 aba Resolution open => fixed
2012-12-10 17:16 aba Changeset attached => occt master f163f612
2013-04-23 13:36 aiv Status verified => closed
2013-04-29 15:21 aiv Fixed in Version => 6.6.0