L1EGammaCrystalsEmulatorProducer writes out of array bounds

[Dr15Jones]

The UBSAN report has

39434.103/step2:L1Trigger/L1CaloTrigger/plugins/L1EGammaCrystalsEmulatorProducer.cc:1046:109: runtime error: index 2 out of bounds for type 'int [2][3]'

and the same report for lines 1048, 1058, 1060, 1062 and out of bounds for an float array at 1050, 1052, 1054, and 1056.

[Dr15Jones]

assign l1

[cmsbuild]

New categories assigned: l1

@epalencia,@rekovic,@cecilecaillol you have been requested to review this Pull request/Issue and eventually sign? Thanks

[cmsbuild]

A new Issue was created by @Dr15Jones Chris Jones.

@Dr15Jones, @perrotta, @dpiparo, @makortel, @smuzaffar, @qliphy can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

[Dr15Jones]

The first reported problem is here

cmssw/L1Trigger/L1CaloTrigger/plugins/L1EGammaCrystalsEmulatorProducer.cc

Lines 1044 to 1047 in c5448e1

	for (int ii = 0; ii < n_towers_halfPhi; ++ii) { // The cluster list is still in the L1 like geometry
	for (unsigned int jj = 0; jj < unsigned(cluster_list_L2[ii].size()) && jj < n_clusters_4link; ++jj) {
	crystalID_cluster_L2Card[n_links_card * (ii % n_clusters_4link) + jj % n_links_card][jj / n_links_card]
	[ii / n_clusters_4link] = cluster_list_L2[ii][jj].ccrystalid_;

it is in the second index used for accessing crystalID_cluster_L2Card which is the calculation jj / n_links_card. The structure is defined here

cmssw/L1Trigger/L1CaloTrigger/plugins/L1EGammaCrystalsEmulatorProducer.cc

Line 779 in c5448e1

int crystalID_cluster_L2Card[n_links_GCTcard][n_clusters_per_link][n_GCTcards];

where the second index is of size

cmssw/L1Trigger/L1CaloTrigger/plugins/L1EGammaCrystalsEmulatorProducer.cc

Line 70 in c5448e1

static constexpr int n_clusters_per_link = 2;

The loop limits are

jj < unsigned(cluster_list_L2[ii].size()) && jj < n_clusters_4link

with

cmssw/L1Trigger/L1CaloTrigger/plugins/L1EGammaCrystalsEmulatorProducer.cc

Line 65 in c5448e1

static constexpr int n_clusters_4link = 4 * 3;

given

cmssw/L1Trigger/L1CaloTrigger/plugins/L1EGammaCrystalsEmulatorProducer.cc

Line 74 in c5448e1

static constexpr int n_links_card = 4;

then the max value of the index used doing the lookup is jj / n_links_card -> n_clusters_4link/n_links_card == 3 which is larger than the array size which is 2.

So it appears the loop end condition of jj < n_clusters_4link is incorrect, and should be something more like jj < n_clusters_per_link* n_links_card.

[cecilecaillol]

Fixed in #37705

[cecilecaillol]

+l1

[cmsbuild]

This issue is fully signed and ready to be closed.