View Issue Details

Related ChangesetsJump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0024489Open CASCADEOCCT:Foundation Classespublic2013-12-21 11:122014-05-12 07:54
ReporterabvAssigned Tobugmaster  
PrioritynormalSeverityminor 
Status closedResolutionfixed 
PlatformLinuxOSMandriva 2008 
Product Version6.7.0 
Target Version6.7.1Fixed in Version6.7.1 
Summary0024489: Avoid type casts in call to Standard::Free()
DescriptionThe method Standard::Free() is made to nullify its argument pointer, in order to protect against possible access to freed memory. Currently this is done in error-prone way: this method accepts reference to Standard_Address, and thus all calls to this method where actual pointer is of different type have to use either temporary variables or type casts. Moreover such type casts of pointers are dangerous as they break strict aliasing rules; this is reported as warning by GCC compiler, see 0024252.

To avoid this problem, Standard::Free() can be made template inline function calling memory deallocation method with void* and then nullifying the pointer using its actual type.
Steps To ReproduceUse GCC compiler with option -Wall
TagsNo tags attached.
Test case numberNot needed

Relationships

related to 0024912 closedapn Community Remove unused Graphic3d_Strips 
child of 0024252 closedbugmaster Open CASCADE GCC warnings on breakage of strict-aliasing rules 

Activities

abv

2013-12-21 13:28

manager   ~0027292

The fix pushed to CR24489, please review

kgv

2013-12-21 21:01

developer   ~0027293

No remarks, please test the patch.

apn

2013-12-23 13:15

administrator   ~0027301

Last edited: 2013-12-23 15:08

 There are compilation errors on Linux:

http://jenkins-test-02.nnov.opencascade.com:8080/job/mnt-CR24489-master_build_occt_linux/1/parsed_console/?

../../../../src/Graphic3d/Graphic3d_Strips.cxx:248: error: no matching function for call to ‘Standard::Free(Graphic3d_Strips::STRIPT_INIT(Standard_Integer, const TColStd_Array1OfInteger&)::edge*&)’
../../../../src/Graphic3d/Graphic3d_Strips.cxx:251: error: no matching function for call to ‘Standard::Free(Graphic3d_Strips::STRIPT_INIT(Standard_Integer, const TColStd_Array1OfInteger&)::edge**&)’
../../../../src/Graphic3d/Graphic3d_Strips.cxx: In static member function ‘static void Graphic3d_Strips::STRIPQ_INIT(Standard_Integer, Standard_Integer, const TColStd_SequenceOfInteger&)’:
../../../../src/Graphic3d/Graphic3d_Strips.cxx:889: error: no matching function for call to ‘Standard::Free(Graphic3d_Strips::STRIPQ_INIT(Standard_Integer, Standard_Integer, const TColStd_SequenceOfInteger&)::edge*&)’
../../../../src/Graphic3d/Graphic3d_Strips.cxx:892: error: no matching function for call to ‘Standard::Free(Graphic3d_Strips::STRIPQ_INIT(Standard_Integer, Standard_Integer, const TColStd_SequenceOfInteger&)::edge**&)’
make[2]: *** [Graphic3d_Strips.lo] Error 1

abv

2013-12-23 14:57

manager   ~0027304

Andrey, could you please post here part of the plain log where the error is reported -- are there any candidates listed there?

abv

2013-12-24 14:34

manager   ~0027322

I have committed new branch under the same name, CR24489, hoping to fix the issue with GCC compiler, please check

apn

2013-12-25 13:54

administrator   ~0027337

Dear BugMaster,

Branch CR24489 (and products from GIT master) was compiled on Linux and Windows platforms and tested.
SHA-1: d19f0326d4ba47ec4cec0f86bba9229dcd8a516c

Number of compiler warnings:

occt component :
Linux: 56 (103 on master)
Windows: 0 (0 on master)

products component :
Linux: 21 (21 on master)
Windows: 1 (1 on master)

Regressions/Differences:
http://occt-tests/CR24489-master-occt/Debian60-64/summary.html
There are debug prints (Backtrace and Memory map) in following test cases on Linux:
blend complex A2 A3 B3 C2 F6
blend encoderegularity A6
blend simple F5 P7 V7 W4 X5 X8
blend tolblend_simple A1 A2 A3 A4 A5 A6 A7 A8 A9 B1 B2 B3 C5 D2 D3 D5 D6 D7 D8 D9 E1 E2 E3 E4 E5 E6 E7 E8 E9 F1 F2 F3 F4 F7 F8
bugs heal bug23944
bugs modalg_1 bug140 bug15836
bugs modalg_2 bug426_1 bug453_1 bug453_2 bug474 bug487 bug22783 bug22786
bugs modalg_4 bug625 bug8842_3 bug8842_7 bug8842_11 bug8842_12
bugs modalg_5 bug23870_1 bug23870_2 bug23870_4 bug23906 bug23952_1
bugs moddata_2 bug50 bug266 bug453_3
chamfer dist_angle_sequence B5 B6 B7 B9
chamfer dist_dist_sequence B5 B6 B7 B9
chamfer equal_dist_sequence B4 B5 B6 B7 B9
heal fix_face_size A1 A2 A3 A4 A5 A7 A8 A9 B2 B3 B4 B5 B6 B7 B8 C1
heal surface_to_bspline A1 A2 A5 A6 A7 A9 B1 B2 B3 B5 B7 B8 B9 C1 C5 C6 C7 D1 D2 D3 D4 D5 D6 D7 D8 E2 E3 E5 E7 E8 E9 F1 F2 F4 F5 F7 F8 G2
offset faces_type_i D7 G5 L1 L2 L3 L4 L5 L6 L7 L8 L9 M1 M2 M3 N2
offset shape_type_i C5 C7 D6 D7 G2 G3 G4

Testing cases:
Not needed

Testing on Linux:
Total MEMORY difference: 354103344 / 355491668
Total CPU difference: 42308.94999999997 / 43355.70000000012

Testing on Windows:
Total MEMORY difference: 412830452 / 412686660
Total CPU difference: 28488.4375 / 34870.640625

There are not differences in images found by testdiff.

abv

2013-12-26 11:41

manager   ~0027350

I have corrected stupid error, and tested on Windows (vc10 x64), please test again

apn

2013-12-27 12:50

administrator   ~0027363

Dear BugMaster,

Branch CR24489 (and products from GIT master) was compiled on Linux and Windows platforms and tested.
SHA-1: dee98ea601cb202997a6020ed03555704a0382be

Number of compiler warnings:

occt component :
Linux: 55 (103 on master)
Windows: 0 (0 on master)

products component :
Linux: 20 (21 on master)
Windows: 1 (1 on master)

Regressions/Differences:
No regressions

Testing cases:
Not needed

Testing on Linux:
Total MEMORY difference: 358476908 / 359763168
Total CPU difference: 43441.69999999996 / 44230.10000000008

Testing on Windows:
Total MEMORY difference: 412608912 / 412672644
Total CPU difference: 32339.765625 / 34870.265625

There are not differences in images found by testdiff.

Roman Lygin

2014-01-17 23:30

updater   ~0027545

Side comment stemming from discussion at 0023855 - what was the motivation to create a template method which likely results in multiple instantiation and hence size bloat ? Why not to simply keep the only function Standard::Free (const Standard_Address), i.e. after removing reference (&), what would be aligned with free() ? Previous signature looked just like a legacy typo.

abv

2014-01-17 23:47

manager   ~0027546

The template function is inline, and its only action is to nullify the pointer being freed, thus I guess size bloat is negligible. The goal is to provide some protection against repetitive freeing the same pointer. At the time of introduction this feature allowed to spot a few errors in OCCT. I still believe it is useful and not a big overhead.

Roman Lygin

2014-01-18 00:38

updater   ~0027547

"The goal is to provide some protection against repetitive freeing the same pointer."

This would be a counter-productive goal in fact. You better expose the issue in a wrong logic code and catch it in then let it rely on mercy of Standard::Free(). That is what free() would do.

abv

2014-01-18 08:09

manager   ~0027548

Sorry Roman, I was wrong in my last comment regarding the kind of problems that are avoided by nullifying the pointer. It is not (only) about freeing the pointer twice, but accessing the freed memory. This can happen either by bad design of the code which accesses pointer after freeing it, or indirectly, when reference to object is kept while the object is destroyed (e.g. using local variable - reference to element of a collection which gets re-initialized during the algorithm). Nullifying the pointer guarantees that this situation will trigger error, while without that it can remain unnoticed.

Related Changesets

occt: master 547702a1

2014-01-09 07:56:20

abv


Committer: bugmaster Details Diff		 0024489: Avoid type casts in call to Standard::Free()

Method Standard::Free() is converted to template, so that pointer is nullified using its proper type.
Unnecessary type cases in calls to Standard::Free(), Standard::Reallocate(), and NCollection_BaseAllocator::Free() eliminated throughout OCCT code.		 Affected Issues
0024489
mod - src/AdvApp2Var/AdvApp2Var_SysBase.cxx Diff File
mod - src/BOPDS/BOPDS_PassKey.lxx Diff File
mod - src/DBC/DBC_VArray.gxx Diff File
mod - src/FSD/FSD_BinaryFile.cxx Diff File
mod - src/Graphic3d/Graphic3d_ArrayOfPrimitives.cxx Diff File
mod - src/Graphic3d/Graphic3d_Strips.cxx Diff File
mod - src/NCollection/NCollection_BaseAllocator.cxx Diff File
mod - src/NCollection/NCollection_LocalArray.hxx Diff File
mod - src/NCollection/NCollection_UtfString.hxx Diff File
mod - src/OpenGl/OpenGl_PrimitiveArray.cxx Diff File
mod - src/Standard/Standard.cdl Diff File
mod - src/Standard/Standard.cxx Diff File
mod - src/Standard/Standard_DefineAlloc.hxx Diff File
mod - src/Standard/Standard_MMgrOpt.cxx Diff File
mod - src/Standard/Standard_MMgrOpt.hxx Diff File
mod - src/Standard/Standard_MMgrRaw.cxx Diff File
mod - src/Standard/Standard_MMgrRaw.hxx Diff File
mod - src/Standard/Standard_MMgrRoot.hxx Diff File
mod - src/Standard/Standard_MMgrTBBalloc.cxx Diff File
mod - src/Standard/Standard_MMgrTBBalloc.hxx Diff File
mod - src/Storage/Storage_Schema.cxx Diff File
mod - src/TCollection/TCollection_Array2.gxx Diff File
mod - src/TCollection/TCollection_ExtendedString.cxx Diff File
mod - src/TopExp/TopExp_Explorer.cxx Diff File
mod - src/XmlObjMgt/XmlObjMgt.cxx Diff File

Issue History

Date Modified Username Field Change
2013-12-21 11:12 abv New Issue
2013-12-21 11:12 abv Assigned To => abv
2013-12-21 11:13 abv Relationship added child of 0024252
2013-12-21 13:28 abv Note Added: 0027292
2013-12-21 13:28 abv Assigned To abv => kgv
2013-12-21 13:28 abv Status new => resolved
2013-12-21 21:01 kgv Note Added: 0027293
2013-12-21 21:01 kgv Assigned To kgv => bugmaster
2013-12-21 21:01 kgv Status resolved => reviewed
2013-12-23 10:49 apn Assigned To bugmaster => apn
2013-12-23 13:15 apn Note Added: 0027301
2013-12-23 13:15 apn Test case number => Not needed
2013-12-23 13:15 apn Assigned To apn => abv
2013-12-23 13:15 apn Status reviewed => assigned
2013-12-23 14:57 abv Note Added: 0027304
2013-12-23 15:08 apn Note Edited: 0027301
2013-12-24 14:34 abv Note Added: 0027322
2013-12-24 14:34 abv Status assigned => resolved
2013-12-24 14:35 abv Assigned To abv => apn
2013-12-24 14:35 abv Status resolved => reviewed
2013-12-25 13:54 apn Note Added: 0027337
2013-12-25 13:55 apn Assigned To apn => abv
2013-12-25 13:55 apn Status reviewed => assigned
2013-12-26 11:41 abv Note Added: 0027350
2013-12-26 11:41 abv Status assigned => resolved
2013-12-26 11:41 abv Assigned To abv => apn
2013-12-26 11:41 abv Status resolved => reviewed
2013-12-27 12:50 apn Note Added: 0027363
2013-12-27 12:50 apn Assigned To apn => bugmaster
2013-12-27 12:50 apn Status reviewed => tested
2014-01-13 10:43 bugmaster Changeset attached => occt master 547702a1
2014-01-13 10:43 bugmaster Status tested => verified
2014-01-13 10:43 bugmaster Resolution open => fixed
2014-01-17 23:30 Roman Lygin Note Added: 0027545
2014-01-17 23:47 abv Note Added: 0027546
2014-01-18 00:38 Roman Lygin Note Added: 0027547
2014-01-18 08:09 abv Note Added: 0027548
2014-05-05 13:36 aiv Status verified => closed
2014-05-05 13:37 aiv Fixed in Version => 6.7.1
2014-05-12 07:54 kgv Relationship added related to 0024912