Merging fuzzing SAPI into core

  106317
July 29, 2019 00:48 smalyshev@gmail.com (Stanislav Malyshev)
Hi!

As you probably know, we've been running PHP fuzzing under Google's
OSS-Fuzz[1] project for a while now (and found and fixed some bugs due
to it).

This has been enabled by the PHP fuzzing API SAPI[2] which currently
lives in a separate repository. Since the setup is working pretty well
for a while now, I would like to propose to merge it into core
repository as a core SAPI, and make Travis CI setup build it as part of
the CI tests.

This would ensure the fuzzing scripts are not broken by core changes
(happened several times recently) and would provide wider exposure to
the fuzzing setup we have, hopefully prompting extension authors and
other contributors to add more fuzzing modules to it, thus enhancing PHP
security and reliability.

Are there any objections or suggestions about this? Do we need an RFC
for it? Note that this is only for master branch (only master is being
fuzzed now), though it would not be hard to port to other branches if
there's interest, the fuzzer should work on pretty much any recent
branch with small code changes.

[1] https://github.com/google/oss-fuzz/
[2] https://github.com/php/php-fuzzing-sapi
-- 
Stas Malyshev
smalyshev@gmail.com
  106318
July 29, 2019 07:39 nikita.ppv@gmail.com (Nikita Popov)
On Mon, Jul 29, 2019 at 2:48 AM Stanislav Malyshev <smalyshev@gmail.com>
wrote:

> Hi! > > As you probably know, we've been running PHP fuzzing under Google's > OSS-Fuzz[1] project for a while now (and found and fixed some bugs due > to it). > > This has been enabled by the PHP fuzzing API SAPI[2] which currently > lives in a separate repository. Since the setup is working pretty well > for a while now, I would like to propose to merge it into core > repository as a core SAPI, and make Travis CI setup build it as part of > the CI tests. > > This would ensure the fuzzing scripts are not broken by core changes > (happened several times recently) and would provide wider exposure to > the fuzzing setup we have, hopefully prompting extension authors and > other contributors to add more fuzzing modules to it, thus enhancing PHP > security and reliability. > > Are there any objections or suggestions about this? Do we need an RFC > for it? Note that this is only for master branch (only master is being > fuzzed now), though it would not be hard to port to other branches if > there's interest, the fuzzer should work on pretty much any recent > branch with small code changes. > > [1] https://github.com/google/oss-fuzz/ > [2] https://github.com/php/php-fuzzing-sapi
Sounds good to me. Feel free to submit a PR for review. Nikita
  106351
July 30, 2019 17:28 bishop@php.net (Bishop Bettini)
On Sun, Jul 28, 2019 at 8:48 PM Stanislav Malyshev <smalyshev@gmail.com>
wrote:

> > As you probably know, we've been running PHP fuzzing under Google's > OSS-Fuzz[1] project for a while now (and found and fixed some bugs due > to it). > > This has been enabled by the PHP fuzzing API SAPI[2] which currently > lives in a separate repository. Since the setup is working pretty well > for a while now, I would like to propose to merge it into core > repository as a core SAPI, and make Travis CI setup build it as part of > the CI tests. > > This would ensure the fuzzing scripts are not broken by core changes > (happened several times recently) and would provide wider exposure to > the fuzzing setup we have, hopefully prompting extension authors and > other contributors to add more fuzzing modules to it, thus enhancing PHP > security and reliability. > > Are there any objections or suggestions about this? Do we need an RFC > for it? Note that this is only for master branch (only master is being > fuzzed now), though it would not be hard to port to other branches if > there's interest, the fuzzer should work on pretty much any recent > branch with small code changes. >
I see benefit both ways. On the one hand, having the fuzzer and engine code together removes the effort of integrating the two, which (as you mention) makes it easier to extend and invoke. This is good for core and extension maintainers. On the other, I've found it refreshing working in a slender repo that doesn't have all the history and process rules. This is good for external (non-core and non-extension) collaborators, particularly allowing write access to those who wouldn't want or need write access to engine code. I lean toward the "keep it separate" option, since the process barriers are much smaller. Couldn't we update Travis to clone the repo, run the build/configure steps, then invoke the fuzzer as part of the make test phase? Or, how about the other way: setup Travis on the fuzzing repo to clone php/php-src:master, integrate it, and run? Either way, I don't think an RFC is applicable. To me, the fuzzer is a dev-dependency (in composer parlance) that doesn't surface to users.
  106360
July 30, 2019 20:39 smalyshev@gmail.com (Stanislav Malyshev)
Hi!

> This is good for external (non-core and non-extension) collaborators, particularly allowing write access to those who wouldn't want or need
write access to engine code. Yes, in theory. In practice I don't see too many of those flocking to it, unfortunately. As for process, I think we have enough people (starting with myself) that would gladly guide through any pull req to this particular part if asked, and I don't see anybody ever needing to go through RFCs etc. to contribute there. So the process is probably "make the pull, ping the list" :)
> I lean toward the "keep it separate" option, since the process barriers > are much smaller. Couldn't we update Travis to clone the repo, run the > build/configure steps, then invoke the fuzzer as part of the make test > phase? Or, how about the other way: setup Travis on the fuzzing repo to > clone php/php-src:master, integrate it, and run?
That's an interesting idea. I still like the integrated repo better, since it's easier for the person making changes to ensure everything builds properly instead of being alerted by Travis and go back digging for a third-party repo they may not even be aware of. But this could be an option too. And btw that reminds me that if it'd be integrated, we also need Travis build to support it. Separate thanks for this reminder! -- Stas Malyshev smalyshev@gmail.com
  106361
July 30, 2019 22:15 johannes@schlueters.de (Johannes =?ISO-8859-1?Q?Schl=FCter?=)
On Tue, 2019-07-30 at 13:28 -0400, Bishop Bettini wrote:
> On the other, I've found it refreshing working in a > slender repo that doesn't have all the history and process rules. > This is good for external (non-core and non-extension) collaborators, > particularly allowing write access to those who wouldn't want or need > write access to engine code.
We could grant access to the respective SAPI directory only. Also this is non-production code so even if merged rules can be relaxed. Only rule we have no real flexibility over is the merge requirement. (not observing that would cause trouble for others) I am also not sure having this independent makes it simpler for outsiders to contribute. You maybe ease the submission process, but setting it up becomes harder as you need to clone to repos, put it in the right place, make sure you are in compatible branches (on next PHP 7 like API change this might become relevant, maybe even sooner as it goes into different extension's APIs where we observe BC a bit less) I think merging it into PHP makes the most sense, also for signaling to the outside that we care about security by having fuzzing routines as core part of the thing. johannes
  106362
July 31, 2019 04:20 bishop@php.net (Bishop Bettini)
On Tue, Jul 30, 2019 at 6:15 PM Johannes Schlüter <johannes@schlueters..de>
wrote:

> On Tue, 2019-07-30 at 13:28 -0400, Bishop Bettini wrote: > > On the other, I've found it refreshing working in a > > slender repo that doesn't have all the history and process rules. > > This is good for external (non-core and non-extension) collaborators, > > particularly allowing write access to those who wouldn't want or need > > write access to engine code. > > > I am also not sure having this independent makes it simpler for > outsiders to contribute. You maybe ease the submission process, but > setting it up becomes harder as you need to clone to repos, put it in > the right place, make sure you are in compatible branches (on next PHP > 7 like API change this might become relevant, maybe even sooner as it > goes into different extension's APIs where we observe BC a bit less) > > I think merging it into PHP makes the most sense, also for signaling to > the outside that we care about security by having fuzzing routines as > core part of the thing. >
Fair points, Johannes and Stas. I've no objection to moving the SAPI into php/php-src.