search

cms-gem-daq-project / ctp7_modules

0 stars 13 forks source link

Replace unsafe calls to `stdsprintf` #141

Closed lpetre-ulb closed 4 years ago

lpetre-ulb commented 5 years ago

Brief summary of issue

The rpcsvc program provides one convenience function which is used throughout the ctp7_modules code base: stdsprintf. This leads to 2 main issues:

  1. The stdsprintf implementation is inherently unsafe with with to the types. If the format specifier does not match the variadic argument types consequences can be disastrous.
  2. Functions written inside the ctp7_modules cannot be used elsewhere, e.g. a standalone program.

Types of issue

Expected Behavior

Use a safe formatting library which can also be used outside of the rpcsvc.

Current Behavior

Calls to stdsprintf are unsafe and depend on the `rpcsvc´ daemon.

Proposals

  1. Use the {fmt} library. It is designed to be small, safe and fast. Depending on the compiler the safety is provided at compile time with errors and/or at runtime with exceptions. The dependency is at compile time for the core version which a header-only library so there shouldn't be any issue with the ARM compilation.

  2. Use the stringstream from the standard library. Syntax is however much heavier, especially for inline string formatting.

bdorney commented 5 years ago

Functions written inside the ctp7_modules cannot be used elsewhere, e.g. a standalone program.

Is it the stdsprintf function that makes these unable to be used in a standalone program or the fact that stdsprintf is used in many of the LOGGER calls?

bdorney commented 5 years ago

Functions written inside the ctp7_modules cannot be used elsewhere, e.g. a standalone program.

Is it the stdsprintf function that makes these unable to be used in a standalone program or the fact that stdsprintf is used in many of the LOGGER calls?

Follow-up, and I guess more important, question where is this function defined?

bdorney commented 5 years ago

Use the {fmt} library. It is designed to be small, safe and fast. Depending on the compiler the safety is provided at compile time with errors and/or at runtime with exceptions. The dependency is at compile time for the core version which a header-only library so there shouldn't be any issue with the ARM compilation.

Can you outline how this could be included in our build process? And are any additional packages required? How would working with this on the P5 technical network go (e.g. no access to the outside world).?

lpetre-ulb commented 5 years ago

Is it the stdsprintf function that makes these unable to be used in a standalone program or the fact that stdsprintf is used in many of the LOGGER calls?

Follow-up, and I guess more important, question where is this function defined?

This is the stdsprintf function itself which is the problem. It is certainly used in many LOGGER calls but also in many more places for producing the register names. This function is defined in the rpcsvc program source code

Use the {fmt} library. It is designed to be small, safe and fast. Depending on the compiler the safety is provided at compile time with errors and/or at runtime with exceptions. The dependency is at compile time for the core version which a header-only library so there shouldn't be any issue with the ARM compilation.

Can you outline how this could be included in our build process? And are any additional packages required? How would working with this on the P5 technical network go (e.g. no access to the outside world).?

Unfortunately, I don't know in detail how and with which constraints packages are build for P5. (If you have some documentation I'll be pleased to read it) However if the Git repository is available on the build nodes it should not be a problem.

The first important thing is that there is no dependency at runtime (except the libc++) so no need for new package on the CTP7. At compile time there is no external dependency either, only the few {fmt} header files are required.

While it would be possible to produce a devel package it is much easier to simply copy the library files at a known location. I think {fmt} is even designed to work that way (but with CMake). I would say that xhal is a good place for it since the xcompile directory already stores libraries used on the CTP7.

You can chose between using a Git submodule and a simple source code copy-paste. I would advise against the Git submodule though since the GitHub repository might disappear over the experiment lifetime or be inaccessible inside the P5 build network.

jsturdy commented 5 years ago

Proposals

  1. Use the {fmt} library. It is designed to be small, safe and fast. Depending on the compiler the safety is provided at compile time with errors and/or at runtime with exceptions. The dependency is at compile time for the core version which a header-only library so there shouldn't be any issue with the ARM compilation.
  2. Use the stringstream from the standard library. Syntax is however much heavier, especially for inline string formatting.

Prefer standard library options over including random dependencies unless there is a demonstrated need because of a performance bottleneck (but this is an optimization that is unlikely to be critical at this juncture in the development)

bdorney commented 5 years ago

@lpetre-ulb I would suggest you make a function for this repo in include/utils.h and include/utils.cpp that behaves as stdsprintf but addresses the concerns you raise here.

Then all instances of stdsprintf can be replaced with this function. I would prefer the usage to stay the same but I'm open to alternatives.

bdorney commented 5 years ago

@lpetre-ulb I would suggest you make a function for this repo in include/utils.h and include/utils.cpp that behaves as stdsprintf but addresses the concerns you raise here.

Then all instances of stdsprintf can be replaced with this function. I would prefer the usage to stay the same but I'm open to alternatives.

Your function should follow from @jsturdy's comments; e.g. using STL rather than external dependency.

jsturdy commented 5 years ago

Any modules that I've added, I've avoided (and actually began removing) stdsprintf and instead using stringstream constructions.

lpetre-ulb commented 5 years ago

Prefer standard library options over including random dependencies unless there is a demonstrated need because of a performance bottleneck (but this is an optimization that is unlikely to be critical at this juncture in the development)

Indeed using the standard library should be preferred. I proposed the {fmt} library to conserve the same usage as stdsprintf; performances were not taken into account.

After a quick look throughout the code I found many different ways of creating strings:

After this quick review I'm convinced we should chose a standardized method. If we are going to use log4cplus my proposal would be to use stringstream for the logging and std::string concatenation for register name generation (since it can easily be used as temporaries).

@lpetre-ulb I would suggest you make a function for this repo in include/utils.h and include/utils.cpp that behaves as stdsprintf but addresses the concerns you raise here.

Then all instances of stdsprintf can be replaced with this function. I would prefer the usage to stay the same but I'm open to alternatives.

Your function should follow from @jsturdy's comments; e.g. using STL rather than external dependency.

Writing a such function in a type-safe way with the standard library means rewriting the {fmt} library. This a a very long and tricky development which would only reinvent the wheel.

jsturdy commented 5 years ago

After a quick look throughout the code I found many different ways of creating strings:

  • std::string with concatenation operator and stringstream to convert integers to strings
  • std::string with concatenation operator and std::to_string
  • stringstream
  • stdsprintf
  • and maybe other ways I missed

After this quick review I'm convinced we should chose a standardized method. If we are going to use log4cplus my proposal would be to use stringstream for the logging and std::string concatenation for register name generation (since it can easily be used as temporaries).

Indeed.

@lpetre-ulb I would suggest you make a function for this repo in include/utils.h and include/utils.cpp that behaves as stdsprintf but addresses the concerns you raise here. Then all instances of stdsprintf can be replaced with this function. I would prefer the usage to stay the same but I'm open to alternatives. Your function should follow from @jsturdy's comments; e.g. using STL rather than external dependency.

Writing a such function in a type-safe way with the standard library means rewriting the {fmt} library. This a a very long and tricky development which would only reinvent the wheel.

Also agree, I strongly disfavour adding an additional wrapper around something that is not at all cumbersome to do with a stringstream

lpetre-ulb commented 4 years ago

Change implemented in https://github.com/cms-gem-daq-project/ctp7_modules/pull/167.