Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[sanitizer_common] Implement popen, popenve, pclose interceptors

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



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


Event Timeline

mgorny created this revision.Dec 29 2018, 2:20 PM
Herald added subscribers: Restricted Project, llvm-commits, kubamracek. · View Herald TranscriptDec 29 2018, 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.

90 ↗(On Diff #179709)

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

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.Dec 31 2018, 6:24 AM
90 ↗(On Diff #179709)

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

2259 ↗(On Diff #179709)

Is File2addr() producing any useful result?


dvyukov added inline comments.Jan 2 2019, 3:59 AM
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.Jan 2 2019, 5:06 AM

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

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

Updated to use COMMON_INTERCEPTOR_FILE_OPEN with nullptr argument.

dvyukov accepted this revision.Jan 2 2019, 5:40 AM
This revision is now accepted and ready to land.Jan 2 2019, 5:40 AM
mgorny marked an inline comment as done.Jan 2 2019, 6:13 AM
mgorny added inline comments.
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.