SpinPool Fcs pi0 finder fixing a crash

[akioogawa]

Fix crash on StSpinPool/StFcsPi0FinderForEcal when Mudst has no primary vertex.

[plexoos]

Does this new check imply that StMuDst::arrays[muPrimaryVertex] is not a valid pointer to a TClonesArray? It might be better to modify StMuDst::primaryVertex() directly...

[genevb]

The finding which I reported to Akio that prompted this PR was that the TClonesArray is valid, but empty. Howeer, StMuDst::primaryVertex() invokes an unchecked array lookup:

/// return pointer to current primary vertex
static StMuPrimaryVertex* primaryVertex() { return (StMuPrimaryVertex*)arrays[muPrimaryVertex]->UncheckedAt(mCurrVertexId); }

There is no value of mCurrVertexId that is valid to use on an empty array, so the current implementation of StMuDst::primaryVertex() is only safely used after a check that the array has some size.

[plexoos]

I find it a bit strange that we don't see this issue with the empty TClonesArray StMuDst::arrays[muPrimaryVertex] when we select the default vertex in StPicoDstMaker:

star-sw/StRoot/StPicoDstMaker/StPicoDstMaker.cxx

Lines 2528 to 2533 in 6dd0b8e

	if ( mVtxMode == PicoVtxMode::Default ) {
	// Choose the default vertex, i.e. the first vertex
	mMuDst->setVertexIndex(0);
	selectedVertex = mMuDst->primaryVertex();
	}

There seems to be a similar check for at least one vertex but it is enabled only when __TFG__VERSION__ is defined.

star-sw/StRoot/StPicoDstMaker/StPicoDstMaker.cxx

Lines 821 to 826 in 6dd0b8e

	#if defined (__TFG__VERSION__)
	if (! mMuDst->numberOfMcVertices()) { // for MC it might be no Vpd
	#endif /* __TFG__VERSION__ */
	if (!selectVertex()) {

Just curious how the code in StSpinPool/StFcsPi0FinderForEcal is different from what we have in StPicoDstMaker?

[genevb]

From what I can tell, the difference is that StFcsPi0FinderForEcal tries to use the returned pointer to access the object, while StPicoDstMaker does not use this pointer for anything other than a check on whether it is non-zero, doing nothing critical with that information other than a print statement:.

star-sw/StRoot/StPicoDstMaker/StPicoDstMaker.cxx

Line 834 in 6dd0b8e

LOG_INFO << "Vertex is not valid" << endm;

[genevb]

Ah, it appears to also decide whether to build the pico event based on the returned pointer evaluating to true or false. The picoDst code doesn't appear robust for events without a primary vertex either to my eyes.

[genevb]

Actually, the array may even get deleted and rebuilt each event?

https://github.com/star-bnl/star-sw/blob/main/StRoot/StMuDSTMaker/COMMON/StMuDstMaker.cxx#L256

Just thinking hypothetically (I didn't check this), I don't see anywhere that mCurrVertexId gets reset between events. What if a new array is instantiated with 16 zeroed elements, but mCurrVertexId is greater than 16 from the previous event? Maybe that's the issue in this particular case?

[genevb]

Nope....I put in a print statement and found that mCurrVertexId is -2 all the way through the crash. Since -2 is not a valid index and goes unchecked, its understandable that it doesn't work to use it as an index, but still not clear why it ever does work.

[plexoos]

but still not clear why it ever does work.

Accessing an array element by index outside of the allocated space is a UB (undefined behavior) in C++. Anything can happen, even correct results can be produced (sometimes).

In the Akio's code a request for primary vertex is ill formed because the vertex index is not specified. The way muDst is coded now, it is the responsibility of the caller to pick a valid vertex. I've checked with the test command posted above that setting it to 0 passes the test. It makes more sense to set the default vertex id in muDst back to 0 rather than -2 and call collectVertexTracks() in the setter unconditionally.

[jdbrice]

So it sounds like I should implement this change instead of (or in addition to) the proposed change here.

[plexoos]

Hold on... I think a somewhat better fix for this particular problem is to modify StFcsPi0FinderForEcal as:

-StMuPrimaryVertex* mutpcvtx=StMuDst::primaryVertex();
+StMuPrimaryVertex* mutpcvtx=StMuDst::primaryVertex(0);

or

+StMuDst::setVertexIndex(0);
 StMuPrimaryVertex* mutpcvtx=StMuDst::primaryVertex();

it is "better" because it is consistent with how it is done in PicoDst where we don't check for the number of vertices in the array.

I am about to add the test using the data file as provided by Gene. Using it I want to check if we see the issue and the proposed solutions fix it.

As for the default vertex id = -2 in MuDst... yeah, but to me it looks like a secondary issue now w.r.t. other priorities 😄