Problem 1255

Summary:	Pointer is not checked for NULL return value
Product:	Geant4	Reporter:	Clayton Davis <claytongdavis>
Component:	event	Assignee:	asai
Status:	RESOLVED FIXED
Severity:	minor	CC:	bloer
Priority:	P5
Version:	other
Hardware:	All
OS:	All

Description Clayton Davis 2011-10-12 02:43:39 CEST

Hi Geant folks,

In the geant4 9.5-beta distribution, in G4SPSPosDistribution.cc, line 957, the function G4SPSPosDistribution::IsSourceConfined calls:

theVolume=gNavigator->LocateGlobalPointAndSetup(particle_position,ptr,true);

It immediately dereferences theVolume.  However, LocateGlobalPointAndSetup can return a NULL pointer; this needs to be checked before dereferencing, and some appropriate action taken besides crashing.

By inspecting the code, this bug also seems to be present in:

visualization/modeling/src/G4TrajectoryDrawByOriginVolume.cc line 64
visualization/modeling/src/G4TrajectoryOriginVolumeFilter.cc line 52
tracking/src/G4SteppingManager.cc line 302

However, I did not specifically trigger these instances, and so I can't guarantee that the pointer is not protected some other way (ie a position that is guaranteed to lie within the volume).

Thanks,
Clayton

Comment 1 asai 2012-07-11 18:34:19 CEST

Hello,

We believe these uses are safe without protecting against null pointer, as they all are invoked during the tracking time, i.e. tracks are already confirmed to be in the world volume. The method LocateGlobalPointAndSetup90 returns null pointer only if the particle position is outside of the world volume.

Kind regards,
Makoto

Comment 2 bloer 2014-02-21 20:53:08 CET

(In reply to comment #1)
> Hello,
> 
> We believe these uses are safe without protecting against null pointer, as they
> all are invoked during the tracking time, i.e. tracks are already confirmed to
> be in the world volume. The method LocateGlobalPointAndSetup90 returns null
> pointer only if the particle position is outside of the world volume.
> 
> Kind regards,
> Makoto


I'd like to request that this be reconsidered. The null dereferencing in G4SPSPosDistribution _is_ causing segfaults in my program. The function in SPSPosDistribution is called before tracking, to check whether a proposed point generated by the gps is inside a named volume.  If the allowed extent is outside the world volume, this returns null. A relevant stacktrace from geant 4.9.6.p01 on SLC5 is below:

The lines below might hint at the cause of the crash.
If they do not help you then please submit a bug report at
http://root.cern.ch/bugs. Please post the ENTIRE stack trace
from above as an attachment in addition to anything else
that might help us fixing this issue.
===========================================================
#8  0x0000003fb049c99e in std::basic_string<char, std::char_traits<char>, std::allocator<char> >::basic_string () from /usr/lib64/libstdc++.so.6
#9  0x00002b6ab283d916 in G4SPSPosDistribution::IsSourceConfined ()
   from /usr/local/cern/geant4/geant4.9.6.p01/lib64/libG4event.so
#10 0x00002b6ab2843164 in G4SPSPosDistribution::GenerateOne ()
   from /usr/local/cern/geant4/geant4.9.6.p01/lib64/libG4event.so
#11 0x00002b6ab2846193 in G4SingleParticleSource::GeneratePrimaryVertex ()
   from /usr/local/cern/geant4/geant4.9.6.p01/lib64/libG4event.so
#12 0x00002b6aafaa3ece in vetotankPrimaryGeneratorAction::GeneratePrimaries (
    this=0x17a7f950, anEvent=0x30) at src/vetotankPrimaryGeneratorAction.cc:62
#13 0x00002b6ab25dd1fb in G4RunManager::GenerateEvent ()
   from /usr/local/cern/geant4/geant4.9.6.p01/lib64/libG4run.so
#14 0x00002b6ab25daf64 in G4RunManager::ProcessOneEvent ()
   from /usr/local/cern/geant4/geant4.9.6.p01/lib64/libG4run.so
#15 0x00002b6ab25da633 in G4RunManager::DoEventLoop ()
   from /usr/local/cern/geant4/geant4.9.6.p01/lib64/libG4run.so
#16 0x00002b6ab25dbef2 in G4RunManager::BeamOn ()
   from /usr/local/cern/geant4/geant4.9.6.p01/lib64/libG4run.so
#17 0x00002b6ab25e3754 in G4RunMessenger::SetNewValue ()
   from /usr/local/cern/geant4/geant4.9.6.p01/lib64/libG4run.so
#18 0x00002b6ab5a4afab in G4UIcommand::DoIt ()
   from /usr/local/cern/geant4/geant4.9.6.p01/lib64/libG4intercoms.so
#19 0x00002b6ab5a5a200 in G4UImanager::ApplyCommand ()
   from /usr/local/cern/geant4/geant4.9.6.p01/lib64/libG4intercoms.so
#20 0x00002b6ab5a38f79 in G4UIbatch::ExecCommand ()
   from /usr/local/cern/geant4/geant4.9.6.p01/lib64/libG4intercoms.so
#21 0x00002b6ab5a3a0be in G4UIbatch::SessionStart ()
   from /usr/local/cern/geant4/geant4.9.6.p01/lib64/libG4intercoms.so
#22 0x00002b6ab5a56f45 in G4UImanager::ExecuteMacroFile ()
   from /usr/local/cern/geant4/geant4.9.6.p01/lib64/libG4intercoms.so
#23 0x00002b6ab5a51d68 in G4UIcontrolMessenger::SetNewValue ()
   from /usr/local/cern/geant4/geant4.9.6.p01/lib64/libG4intercoms.so
#24 0x00002b6ab5a4afab in G4UIcommand::DoIt ()
   from /usr/local/cern/geant4/geant4.9.6.p01/lib64/libG4intercoms.so
#25 0x00002b6ab5a5a200 in G4UImanager::ApplyCommand ()
   from /usr/local/cern/geant4/geant4.9.6.p01/lib64/libG4intercoms.so
#26 0x0000000000405413 in main (argc=3, argv=0x7fff707e2788) at vetotank.cc:97
===========================================================

Comment 3 asai 2014-02-21 21:43:03 CET

I see. I agree we need a protection in G4SPSPosDistribution::IsSourceConfined(). I fix it. For other places you pointed in your earlier mail, I still believe they are safe.
This fix will not be included in Geant4 v10.0-p01, that will be released very soon, but will be included in the next patch (v10.0-p02).
Thanks for reporting this,
Makoto