Page MenuHomePhabricator

[dfsan] Add wrappers for v*printf functions
ClosedPublic

Authored by gbalats on Jul 16 2021, 2:59 PM.

Details

Summary

Functions vsnprintf, vsprintf and vfprintf commonly occur in DFSan warnings.

Diff Detail

Event Timeline

gbalats requested review of this revision.Jul 16 2021, 2:59 PM
gbalats created this revision.
Herald added a subscriber: Restricted Project. · View Herald TranscriptJul 16 2021, 2:59 PM
compiler-rt/test/dfsan/custom.cpp
1948

We added test cases here to verify these functions' behavior.
Although internally we know it does the same as others, it would still be good to apply some tests. Can reusing sprintf's test code reduce duplicated test code?

vfprintf is actually a potential output point, and can be considered as a sink.

It is like __dfsw_write, and needs a callback so that users can decide whether to take this as a sink.

stephan.yichao.zhao edited the summary of this revision. (Show Details)Jul 16 2021, 4:15 PM
gbalats added inline comments.Jul 16 2021, 4:26 PM
compiler-rt/test/dfsan/custom.cpp
1948

This is what I started with but it ended up either over-complicating the test or adding duplicated code. Do you expect it's ever possible for the implementations of sprintf and vsprintf to diverge?

compiler-rt/test/dfsan/custom.cpp
1948
int sprintf_ish( char * s, const char * format, ... )
{
  va_list args;
  va_start (args, format);
  int r = vsprintf (s ,format, args);
  va_end (args);
  return r;
}

// When testing vsprintf
#define PRINT sprintf_ish
// When testing sprintf
#define PRINT sprintf

sprintf_ish is like a conversion from ... to va_list.
The existing sprintf test code needs a function like this kind of signature. I am not sure if in C we can define a function pointer with ... argument.
If not, we can replace those sprintf in the current test code by PRINT, and define PRINT differently when testing sprintf and vsprintf.
If function pointer works, then we can carry on a pointer to sprintf_ish.

compiler-rt/test/dfsan/custom.cpp
1948

re "Do you expect ... to diverge?": I guess no, while testing them mainly protects dfsan_custom.cpp in case anyone changes them in the future.

gbalats marked 2 inline comments as done.Jul 22 2021, 1:25 PM
gbalats added inline comments.
compiler-rt/test/dfsan/custom.cpp
1948

Tried to share the testing code by introducing a function pointer and then passing either s(n)printf or vs(n)printf respectively. However, this seems to hit a DFSan limitation by being unable to handle indirect calls to variable-argument functions:

==57096==FATAL: DataFlowSanitizer: unsupported indirect call to vararg function snprintf

The macro idea would require all the testing code for these functions to be turned into a macro as well so that we can instantiate it twice, which would be very clunky.

stephan.yichao.zhao accepted this revision.EditedJul 22 2021, 1:40 PM

Please remove vfprintf from the change's description or add a TODO in vfprintf about adding a callback to track it as an output point.

This revision is now accepted and ready to land.Jul 22 2021, 1:40 PM
gbalats marked an inline comment as done.Jul 22 2021, 2:48 PM

vfprintf is actually a potential output point, and can be considered as a sink.

It is like __dfsw_write, and needs a callback so that users can decide whether to take this as a sink.

Isn't this the same for fprintf then which is also discarded currently?

vfprintf is actually a potential output point, and can be considered as a sink.

It is like __dfsw_write, and needs a callback so that users can decide whether to take this as a sink.

Isn't this the same for fprintf then which is also discarded currently?

If we wanted to track output accurately, maybe fprintf also needs a callback. But this is out of the scope of this change.

This revision was automatically updated to reflect the committed changes.