Use of `sprintf` in Trilinos

[cwpearson]

Question

I submitted two PRs to clean up warnings in builds I frequently use:

• https://github.com/trilinos/Trilinos/pull/11879
• https://github.com/trilinos/Trilinos/pull/11878

It was suggested (@GrahamBenHarper) that this might warrant a broader discussion.

sprintf operates on null-terminated strings without bounds checking. For this reason, it is broadly considered sketchy. c++11 introduced a replacement snprintf which takes a additional parameter which specifies the maximum number of bytes (-1, to facilitate null-termination) that will be written out to the destination.

A simple example: This writes x to buf formatted as %d. The danger here is:

• if formatting x is 100 bytes long or more, sprintf will write OOB.

  char buf[100];
  std::sprintf(buf, "%d", x);

This would be replaced with

char buf[100];
std::snprintf(buf, sizeof(buf), "%d", x);

Other uses of sprintf are to format strings into strings. This puts s0 and s1 into buf with a space between them. This has the same problem as above, plus an additional problem: if s0 or s1 were improperly null-terminated, we'll be reading them out of bounds.

char buf[100];
std::sprintf(buf, "%s %s", s0, s1);

This problem is a bit broader and not really solved by snprintf.

In my opinion we should be rejecting any new code that uses c-style strings, and it should be reworked to use std::string and std::string_view as appropriate. I understand there's a lot of old (unmaintained?, working) code that uses sprintf that may be hard to change. I plan to do some more drive-by fixes to the first case as they cause warnings in builds I care about. Is there a broader strategy to adopt?

[jhux2]

@ccober6

[jhux2]

@trilinos/framework

[ccober6]

@cwpearson thanks for bringing this up! I agree that we should clean this up across Trilinos. We have a list of technical-debt items. I will add this to the list. There has been some discussion about tackling these "technical-debt" items, but no timeline at this point. I would suggest continuing "drive-by" fixes, as it is at least some improvement. :)

[github-actions[bot]]

This issue has had no activity for 365 days and is marked for closure. It will be closed after an additional 30 days of inactivity. If you would like to keep this issue open please add a comment and/or remove the MARKED_FOR_CLOSURE label. If this issue should be kept open even with no activity beyond the time limits you can add the label DO_NOT_AUTOCLOSE. If it is ok for this issue to be closed, feel free to go ahead and close it. Please do not add any comments or change any labels or otherwise touch this issue unless your intention is to reset the inactivity counter for an additional year.