Page MenuHomePhabricator

[sanitizer_common] Implement popen, popenve, pclose interceptors
ClosedPublic

Authored by mgorny on Sat, Dec 29, 2:20 PM.

Details

Summary

Implement the interceptors for popen(), pclose() and popenve()
functions. The first two are POSIX, the third one is specific
to NetBSD. popen() spawns a process and creates a FILE object piping
data from/to that process. pclose() closes the pipe and waits for
the process to terminate appropriately.

For the purpose of popen(), a new COMMON_INTERCEPTOR_PIPE_OPEN macro is
added that is like COMMON_INTERCEPTOR_FILE_OPEN, except it is passed
the new FILE* and no path.

Diff Detail

Repository
rL LLVM

Event Timeline

mgorny created this revision.Sat, Dec 29, 2:20 PM
Herald added subscribers: Restricted Project, llvm-commits, kubamracek. · View Herald TranscriptSat, Dec 29, 2:20 PM

Looks good to me, but it would be nice to have opinion of @vitalybuka and/or @dvyukov (TSan).

This needs some tests, at least to exercise interceptors code once.

lib/esan/esan_interceptors.cpp
90 ↗(On Diff #179709)

The idea behind defining a no-op version in sanitizer_common_interceptors.inc was exactly that tools that are not interested in it does not need to be changed. So please remove this.

lib/tsan/rtl/tsan_interceptors.cc
2259 ↗(On Diff #179709)

An alternative would be to pass NULL to COMMON_INTERCEPTOR_FILE_OPEN and then do:

if (path)
  Acquire(thr, pc, File2addr(path))

Just to not multiply entities. But neither approach looks strictly better to me, so I am not too strong about it.

krytarowski added inline comments.Mon, Dec 31, 6:24 AM
lib/esan/esan_interceptors.cpp
90 ↗(On Diff #179709)

I would add an intermediate revision to drop redefinitions in this file.

lib/tsan/rtl/tsan_interceptors.cc
2259 ↗(On Diff #179709)

Is File2addr() producing any useful result?

Reusing COMMON_INTERCEPTOR_FILE_OPEN looks fine.

dvyukov added inline comments.Wed, Jan 2, 3:59 AM
lib/tsan/rtl/tsan_interceptors.cc
2259 ↗(On Diff #179709)

Is File2addr() producing any useful result?

What exactly do you mean?
It produces a degenerate, but useful result. The original idea was that it can return different results for different files, depending on io_sync flag value. E.g. not synchronizing deletion of one file with open of another file. But this gets tricky with links, bind mounts, etc and was never implemented.
But at least if a file is not involved at all, then we need to skip the Acquire.

mgorny marked 4 inline comments as done.Wed, Jan 2, 5:06 AM

The tests were submitted earlier as D56153. I'll implement the remaining suggestions.

mgorny updated this revision to Diff 179842.Wed, Jan 2, 5:39 AM

Updated to use COMMON_INTERCEPTOR_FILE_OPEN with nullptr argument.

dvyukov accepted this revision.Wed, Jan 2, 5:40 AM
This revision is now accepted and ready to land.Wed, Jan 2, 5:40 AM
mgorny marked an inline comment as done.Wed, Jan 2, 6:13 AM
mgorny added inline comments.
lib/esan/esan_interceptors.cpp
90 ↗(On Diff #179709)

@krytarowski, apparently it's not that simple. I've tried removing all the empty definitions and now I get a lot of build errors ;-). It would probably be necessary to take them one by one to establish which one are unnecessary after all, and I don't think we have time for this right now.

This revision was automatically updated to reflect the committed changes.