Skip to content
This repository was archived by the owner on Feb 16, 2026. It is now read-only.

Add single-process and fuzzer mode - #3152

Closed
fwsGonzo wants to merge 3 commits into
varnishcache:masterfrom
fwsGonzo:single_process
Closed

fwsGonzo wants to merge 3 commits into
varnishcache:masterfrom
fwsGonzo:single_process

Conversation

@fwsGonzo

Copy link
Copy Markdown
Contributor

Adds separate SINGLE_PROCESS_MODE and LIBFUZZER_ENABLED. Single-process mode allows you to run varnishd in a single process, which will let you use things like PGO and gprof profiling, but is also required for libfuzzer. Libfuzzer mode makes it so that the varnishd main function returns in a manner which does not call any exit functions. This allows for initializing varnishd once, and then fuzzing against (for example) UNIX sockets shortly after.

@dridi
dridi self-requested a review December 10, 2019 17:02

@dridi dridi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed this offline with @fwsGonzo and while in general I like this better than the previous attempt I still have minor nitpicks.

I would like to see 3 patches to make the review easier:

  • one that moves some logic to the child_startup() function
  • a second one for the single process mode
  • and finally one for libfuzzer integration

Regarding single process mode and libfuzzer integration, I notice a distinct lack of autoconf involvement, I can help or deal with that. But first, the nitpicks.

Comment thread bin/varnishd/mgt/mgt_child.c Resolved
Comment thread bin/varnishd/mgt/mgt_child.c Resolved
Comment thread bin/varnishd/mgt/mgt_child.c Resolved
Comment thread bin/varnishd/mgt/mgt_main.c Resolved
Comment on lines +888 to 894
#ifdef LIBFUZZER_ENABLED
return u;
#endif

if (eric_fd >= 0)
mgt_eric_im_done(eric_fd, u);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With libfuzzer enabled this early return leaves dead code, there might be a more elegant solution.

@fwsGonzo

fwsGonzo commented Dec 30, 2019 •

Copy link
Copy Markdown
Contributor Author

I want to make some changes to panic code when libfuzzer is enabled, but I haven't had time to look at it yet. I don't know what varnish does to handle panics, but I think it might be interfering with the libfuzzer sig handler.

Having said that ... it might be preferrable to not do anything about it as well. Right now we generate coredumps from crashes, which are probably way more useful. Perhaps the solution here is to do nothing.

@dridi

dridi commented Dec 30, 2019

Copy link
Copy Markdown
Member

We either panic explicitly, or have some signal handlers turn signals like SIGSEGV into panics...

@dridi

dridi commented Dec 30, 2019

Copy link
Copy Markdown
Member

Regarding core dumps we have the no_coredump feature flag to disable them.

@dridi

dridi commented Dec 30, 2019

Copy link
Copy Markdown
Member

Bugwash consensus: let's try with a debug flag instead of #ifdef guards.

@fwsGonzo you can add the following to your single process mode patch:

diff --git a/include/tbl/debug_bits.h b/include/tbl/debug_bits.h
index f76eb76ec..915e90505 100644
--- a/include/tbl/debug_bits.h
+++ b/include/tbl/debug_bits.h
@@ -51,6 +51,7 @@ DEBUG_BIT(PROCESSORS,         processors,     "Fetch/Deliver processors")
 DEBUG_BIT(PROTOCOL,            protocol,       "Protocol debugging")
 DEBUG_BIT(VCL_KEEP,            vcl_keep,       "Keep VCL C and so files")
 DEBUG_BIT(LCK,                 lck,            "Additional lock statistics")
+DEBUG_BIT(SINGLE_PROC,         single_proc,    "Run in single process mode")
 #undef DEBUG_BIT
 
 /*lint -restore */

Then it's "only" a matter of doing this:

-#ifndef SINGLE_PROCESS_MODE
+if (!MGT_DO_DEBUG(DBG_SINGLE_PROC)) {
 /* ... */
-#endif
+}

Indent, rinse, repeat... This way as @bsdphk suggested we can have at least some basic coverage in a test case:

varnish v1 -cliok "param.set debug +single_proc"
varnish v1 -vcl+backend {...} -start

And the test case may do a few things like a couple transactions, stopping the child, restarting the child...

@fwsGonzo
fwsGonzo force-pushed the single_process branch 4 times, most recently from 3ee2eee to 59acc9b Compare January 2, 2020 10:40
@dridi

dridi commented Jan 2, 2020

Copy link
Copy Markdown
Member

One thing we discussed about the single process mode is whether we want it to be "feature-complete".

For example, is it OK that once the child is started as a thread we can't stop and start it again? @fwsGonzo's use case is only libfuzzer support and we don't have such a request from anyone else. This also also breaks process isolation altogether and if we are going for a debug bit we can probably live with the limitation?

On the other hand in order to have some minimal coverage in a VTC to make sure we don't accidentally break single process mode in the future, we also need varnishd to shut down properly.

@fwsGonzo
fwsGonzo force-pushed the single_process branch 4 times, most recently from f1605f4 to dd6e334 Compare January 2, 2020 15:14
@dridi

dridi commented Jan 2, 2020

Copy link
Copy Markdown
Member

I did more digging with @fwsGonzo and came to the conclusion that this isn't really single process mode that we need to use libfuzzer. It's rather a sort of "exec" mode where the manager leaves its place to the child and there's no coming back.

One thing I couldn't ensure once we have more than one thread in the manager is that VEV events get delivered to the manager's thread. That would mean having to do ALL the setup BEFORE the child is started in this mode.

@bsdphk

bsdphk commented Mar 13, 2020

Copy link
Copy Markdown
Contributor

I'm not keen on this patch for a number of reasons.

Thinking a bit more "high-level" about it, maybe the entire "launch the child excution context" should be a VJ function, so we could have a "single-process" VJ useful for fuzzing, other analysis tools, running debuggers etc.

@bsdphk

bsdphk commented Nov 16, 2020

Copy link
Copy Markdown
Contributor

Bugwash: This ticket has served its purpose.

The way forward is something along the lines of VJ_launch_child() and until somebody prototypes that, it is a bit hard to guess precisely what the contour of that should be.

@bsdphk bsdphk closed this Nov 16, 2020
dridi added a commit that referenced this pull request Aug 30, 2021
Working on the workspace sanitizer (ancestor of the workspace emulator)
final rollbacks were needed to unwind allocations. There was however a
branch where error handling was missing a workspace release, and it was
fine before the introduction of the final rollbacks.

To avoid turning the workspace emulator into a DoS vector the rollbacks
are now only enforced for emulator builds. The specific "insufficient
workspace" log is amended to ensure future changes to the session
workspace footprint don't accidentally remove test coverage for that
branch. The same could be done for other "insufficient workspace" logs
in the PROXY protocol parsing.

Refs 0632b84 (req: Prevent early rollback)
Refs ce71896 (sess: Plug conceptual leak)
Refs 246b1eb (busyobj: Plug conceptual leak)
Refs 5b4f0f1 (htc: Defer workspace rollbacks for request tasks)
Refs #3644

Spotted by Alf's single process fuzzing setup that we should eventually
revisit.

Refs #3152
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants