Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DAQ: fix FedRawData source going out of array boundaries(11_1_X backport) #32121

Merged
merged 4 commits into from
Nov 15, 2020

Conversation

smorovic
Copy link
Contributor

PR description:
Fix FRD format array version check. Version 6 did not get added to the version size mapping, but was allowed in modules, leading to reading out of boundaries.
This by luck did not cause any problems so far on standard gcc builds on x86_64, but caused assertion in IB tests in for CLANG build, as well as ARM and PPC builds where it was spotted (see #32091)

Fix also removes hardcoded version from around the code. It is now defined only in the same header file where it also defines std::array size.

Bugfix doesn't change any other behavior.
Note: it does NOT affect anything used in production DAQ, only version 5 FRD is used there and that is not affected.

PR validation:
Fixes unit test in EventFilter/Utilities/test for CLANG build.

if this PR is a backport please specify the original PR and why you need to backport that PR:

Backport of #32116 (master)

mapping, but was allowed in modules, leading to reading out of
boundaries. Hardcoded version is no longer used now except in header
where it also defines array size.
@cmsbuild
Copy link
Contributor

A new Pull Request was created by @smorovic (Srecko Morovic) for CMSSW_11_1_X.

It involves the following packages:

EventFilter/Utilities
IOPool/Streamer

@perrotta, @smuzaffar, @Dr15Jones, @makortel, @emeschi, @cmsbuild, @slava77, @jpata, @smorovic can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @makortel, @wddgit this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@smorovic
Copy link
Contributor Author

please test for slc7_aarch64_gcc820

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.
Test Parameters:

  • ARCHITECTURE_FILTER = slc7_aarch64_gcc820

@smorovic
Copy link
Contributor Author

please test for slc7_ppc64le_gcc820

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 12, 2020

The tests are being triggered in jenkins.
Test Parameters:

@mrodozov
Copy link
Contributor

oh wait we don't have 11_1 ppc

@mrodozov
Copy link
Contributor

mrodozov commented Nov 12, 2020

I wasn't seeing the PR number is different. There is no PPC release for this. Sorry

@mrodozov
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 12, 2020

The tests are being triggered in jenkins.

@smorovic
Copy link
Contributor Author

please test for CMSSW_11_1_CLANG_X

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.
Test Parameters:

  • RELEASE_FORMAT = CMSSW_11_1_CLANG_X

@cmsbuild
Copy link
Contributor

-1
Tested at: UNKNOWN
I was not able to find a release to test this PR. See the Jenkins logs for more details.

@smorovic
Copy link
Contributor Author

+1
(seems also no CLANG for 11_1)

@makortel
Copy link
Contributor

+1

@perrotta
Copy link
Contributor

+1

  • Identical to the master version
  • Array boundaries fixed as planned
  • jenkins tests pass

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_11_1_X IBs (but tests are reportedly failing) and once validation in the development release cycle CMSSW_11_2_X is complete. This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@silviodonato
Copy link
Contributor

This by luck did not cause any problems so far on standard gcc builds on x86_64

@smorovic so I assume it is not required for the next MWGR

@smorovic
Copy link
Contributor Author

@silviodonato
this is not expected to have any impact on MWGR. today I will do a dry run in Global to check that release 11_1_5 works (just in case).

@silviodonato
Copy link
Contributor

+1

@silviodonato
Copy link
Contributor

merge

@cmsbuild cmsbuild merged commit 2c962ef into cms-sw:CMSSW_11_1_X Nov 15, 2020
@smorovic smorovic deleted the 111X-fix-frdversion-array branch March 8, 2021 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants