Fix various string related errors #21278
Conversation
This resolves compilation errors reported in CMSSW_10_0_UBSAN_X_2017-11-10-1100 Majority of them are `-Werror=format-truncation=`. Either buffer was increased (because few bytes were missing) or type + format was replacemed if we print single digit numbers. In many cases I replaced `sprintf` to `snprintf` with strict buffer boundaries for security. Signed-off-by: David Abdurachmanov <David.Abdurachmanov@cern.ch>
|
The code-checks are being triggered in jenkins. |
|
+code-checks |
|
A new Pull Request was created by @davidlt for master. It involves the following packages: Alignment/CommonAlignmentMonitor @perrotta, @ghellwig, @monttj, @vazzolini, @kmaeshima, @arunhep, @cerminar, @dmitrijus, @cmsbuild, @franzoni, @jfernan2, @slava77, @vanbesien, @lpernie can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
-1 Tested at: 8fcfe2e The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: You can see the results of the tests here: I found follow errors while testing this PR Failed tests: ClangBuild
I found a compilation error while trying to compile with clang: Selected class -> MuScleFitMuon for ROOT: MuScleFitMuon
Selected class -> MuonPair for ROOT: MuonPair
Selected class -> GenMuonPair for ROOT: GenMuonPair
Selected class -> MuScleFitProvenance for ROOT: MuScleFitProvenance
>> Compiling edm plugin /build/cmsbld/jenkins-workarea/workspace/ib-any-integration/CMSSW_10_0_X_2017-11-11-1100/src/DQMOffline/Alignment/src/LaserAlignmentT0ProducerDQM.cc
/build/cmsbld/jenkins-workarea/workspace/ib-any-integration/CMSSW_10_0_X_2017-11-11-1100/src/CalibCalorimetry/EcalCorrelatedNoiseAnalysisAlgos/src/TEcnaParHistos.cc:1232:13: error: variable-sized object may not be initialized
char f_in[fgMaxCar]{0};
^~~~~~~~
/build/cmsbld/jenkins-workarea/workspace/ib-any-integration/CMSSW_10_0_X_2017-11-11-1100/src/CalibCalorimetry/EcalCorrelatedNoiseAnalysisAlgos/src/TEcnaParHistos.cc:1289:13: error: variable-sized object may not be initialized
char f_in[fgMaxCar]{0};
^~~~~~~~
The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
Looks like Clang does not support initialization for variable-size
objects:
error: variable-sized object may not be initialized
Signed-off-by: David Abdurachmanov <David.Abdurachmanov@cern.ch>
|
Pull request #21278 was updated. @perrotta, @ghellwig, @monttj, @vazzolini, @kmaeshima, @arunhep, @cerminar, @dmitrijus, @cmsbuild, @franzoni, @jfernan2, @slava77, @vanbesien, @lpernie can you please check and sign again. |
|
please test |
|
The tests are being triggered in jenkins. |
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
| @@ -188,11 +188,9 @@ void AlignmentMonitorMuonSystemMap1D::book() | |||
| { | |||
| std::string wheel_label[5]={"A","B","C","D","E"}; | |||
|
|
|||
| for (int station = 1; station<=4; station++) | |||
| for (unsigned char station = 1; station<=4; station++) | |||
There was a problem hiding this comment.
@davidlt can you clarify why this change is the preferred way?
There was a problem hiding this comment.
Well, the station seems to be never below 0 thus let's be more explicit about it. It also should allow to catch overflow with static and run-time analyzers. It's also counting just to 4, thus a single char can handle that. Also char is signed or unsigned based on architecture you are compiling thus specifying it explicitly provides more portable code. Also in string formats, like sprintf(c_station, "%d", station);, the compiler would complain that larger buffer is required, because int can hold higher decimal value. At the end, I think, I switched to use std::string instead of char[] so this could be also unsigned int. Also because char only cost 8 bytes you don't need to use full 32-bit register at least on x86_64/amd64, while it has not benefits on aarch64 where minimal register access width is 32-bit. The more you can fit into registers the less spilling and filling you will have, i.e. saving value to memory and loading it later back to do the operation.
I can modify to whichever is preferred.
There was a problem hiding this comment.
ok, understood. there are two changes:
signed -> unsigned (to be explicit)
int -> char (to reduce the buffer size requirement)
I am fine with that change, then.
There was a problem hiding this comment.
Correct and thanks!
| @@ -20,6 +20,7 @@ CastorLedAnalysis::CastorLedAnalysis(const edm::ParameterSet& ps) | |||
| evt=0; | |||
| sample=0; | |||
| m_file=nullptr; | |||
| char output[100]{0}; | |||
There was a problem hiding this comment.
@davidlt why don't you replace this by std::string here, too?
Seems like output is only used as input for an std::ofstream, right?
Similar comments would apply also below, but I understand that you do not want to replace all C string buffers, but only fix the errors in a minimally invasive manner, right?
There was a problem hiding this comment.
Correct, trying to keep it minimal here, somewhat. If you want to do a bigger refactoring it would be better if responsible people/authors do it.
I haven't even looked where char output[] ends up being used at the end.
| @@ -217,7 +217,7 @@ void EcalTB07DaqFormatter::interpretRawData(const FEDRawData & fedData , | |||
|
|
|||
|
|
|||
| short TowerStatus[MAX_TT_SIZE+1]; | |||
| char buffer[20]; | |||
| char buffer[25]; | |||
There was a problem hiding this comment.
this is needed apparently for sprintf(buffer, "FE_CHSTATUS#%d", i);
and the error message is EcalTB07DaqFormatter.cc:224:10: note: 'sprintf' output between 14 and 23 bytes into a destination of size 20
There are 12 characters in the format, "i" can be up to 2 (it's in the range of 1 to 70), and then there is a termination "0" for a total of 15 chars.
What's going on?
Is this added "to be safe just because an arbitrary int can take up to MAX_INT"?
e.g. (impossible in this context) "FE_CHSTATUS#-2147483648" will take 23 chars +1 for termination.
This looks like a false-positive to me and we'd likely have to make these follow-up PRs every time someone uses a similar pattern
There was a problem hiding this comment.
Yes. Compiler calculates the highest possible buffer size based on the types & format specifiers. We cleaned such things in the past, but new flags revealed a few more places we missed. In particularly this would be very bad if sprintf is used. E.g. due to some mistake (a bug) i overflows and then sprintf damages data on the buffer because it overflows buffer.
Technically in this case we can just use std::to_string:
auto blah = "FE_CHSTATUS#" + std::to_string(10);
Note ,this might be interesting also: #21281
|
+1 |
|
ping^1 |
|
+1 |
|
merge |
This resolves compilation errors reported in CMSSW_10_0_UBSAN_X_2017-11-10-1100
Majority of them are
-Werror=format-truncation=. Either buffer wasincreased (because few bytes were missing) or type + format was
replacemed if we print single digit numbers.
In many cases I replaced
sprintftosnprintfwith strict bufferboundaries for security.
Signed-off-by: David Abdurachmanov David.Abdurachmanov@cern.ch