cms-sw / cmssw

CMS Offline Software
http://cms-sw.github.io/
Apache License 2.0
1.09k stars 4.32k forks source link

replace [`std::`]`strerror()` with `strerror_s()` ? #41902

Open fwyzard opened 1 year ago

fwyzard commented 1 year ago

CMSSW makes use of std::strerror() and ::strerror():

$ grep -r '\<strerror *(' */  -l
Alignment/Geners/src/BinaryArchiveBase.cc
CalibCalorimetry/EcalLaserSorting/src/LaserSorter.cc
CalibCalorimetry/EcalLaserSorting/src/WatcherStreamFileReader.cc
CommonTools/UtilAlgos/src/TFileService.cc
DQMServices/FileIO/plugins/DQMFileSaverOnline.cc
DQMServices/FileIO/plugins/DQMFileSaverPB.cc
DataFormats/Scalers/test/ScalersTest.cpp
EventFilter/EcalRawToDigi/plugins/MatacqProducer.cc
EventFilter/Utilities/plugins/FRDOutputModule.cc
EventFilter/Utilities/plugins/GlobalEvFOutputModule.cc
EventFilter/Utilities/plugins/RawEventFileWriterForBU.cc
EventFilter/Utilities/src/DAQSource.cc
EventFilter/Utilities/src/EvFDaqDirector.cc
EventFilter/Utilities/src/FedRawDataInputSource.cc
FWCore/ParameterSet/src/ConfigurationDescriptions.cc
FWCore/Services/plugins/InitRootHandlers.cc
GeneratorInterface/LHEInterface/plugins/ExternalLHEProducer.cc
HLTrigger/Timer/plugins/FastTimerService.cc
HLTrigger/JSONMonitoring/plugins/HLTriggerJSONMonitoring.cc
HLTrigger/JSONMonitoring/plugins/L1TriggerJSONMonitoring.cc
IOPool/Streamer/src/StreamerFileIO.cc
RecoTracker/MkFitCore/src/TrackerInfo.cc
Utilities/StorageFactory/src/LocalCacheFile.cc
Utilities/StorageFactory/src/LocalFileSystem.cc
Utilities/StorageFactory/src/Throw.cc
Utilities/StorageFactory/test/mkstemp.cpp

According to the C and C++ librariy documentation (e.g. https://en.cppreference.com/w/cpp/string/byte/strerror )

strerror is not required to be thread-safe. Implementations may be returning different pointers to static read-only string literals or may be returning the same pointer over and over, pointing at a static buffer in which strerror places the string.

Would it be useful to replace our usage of strerror() with strerror_r(), which uses a user-provided buffer and is thread-safe ?

fwyzard commented 1 year ago

assign core

cmsbuild commented 1 year ago

New categories assigned: core

@Dr15Jones,@smuzaffar,@makortel you have been requested to review this Pull request/Issue and eventually sign? Thanks

cmsbuild commented 1 year ago

A new Issue was created by @fwyzard Andrea Bocci.

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

cms-bot commands are listed here

makortel commented 1 year ago

Thanks @fwyzard for raising the issue.

Would it be useful to replace our usage of strerror() with strerror_r(), which uses a user-provided buffer and is thread-safe ?

I agree, we should review each case and maybe see first if the code could be changed to not use errno() (e.g. TFileService.cc looked like that), and if not, then move to strerror_r(). A potential downside is that while strerror() is part of C and C++ standards, strerror_r() is POSIX` (on the other hand we depend on POSIX in many other places already so the added risk is likely negligible).