Opened 5 years ago

Closed 5 years ago

#563 closed Bug / Defect (fixed)

OpenVPN 2.3.7: using --management-hold causes PID file to not be written

Reported by: kristov Owned by: Steffan Karger
Priority: minor Milestone: release 2.3.8
Component: Generic / unclassified Version: OpenVPN 2.3.7 (Community Ed)
Severity: Not set (select this one, unless your'e a OpenVPN developer) Keywords:
Cc:

Description

Since commit da9b292733e929a2900dc32d37f0424c3d588366 (f025de005d719201a69ad0313d545a1ddd244752 in release/2.3), if the OpenVPN daemon is startet with --management-hold, its PID file is not written. This is a regression to earlier versions which did write the PID file. I tested OpenVPN 2.3.7 (where the PID file is not written), and OpenVPN 2.3.6 as well as OpenVPN 2.3.7 with commit f025de005d719201a69ad0313d545a1ddd244752 being reverted, which both write the PID file. Looking at the changeset, it seems that the removal of calling do_init_first_time() (which creates the PID file) from within do_hold() leads to the erroneous behaviour. The PID file does get written as soon as the HOLD mode is left and the tunnel is started, but that's too late.

The bug has been detected in the context of the fli4l router project (http://www.fli4l.de/en/), where OpenVPN is provided as the main tunnelling solution. The fli4l OpenVPN scripts use the PID file to determine whether the OpenVPN daemon is running before talking to it via the management port.

Attachments (2)

0001-fix-management-hold.patch (1.4 KB) - added by kristov 5 years ago.
fixes management-hold
0001-write-pid-file-immediately-after-daemonizing.patch (4.2 KB) - added by Steffan Karger 5 years ago.

Download all attachments as: .zip

Change History (8)

Changed 5 years ago by kristov

fixes management-hold

comment:1 Changed 5 years ago by kristov

Reverting the pieces of commit f025de005d719201a69ad0313d545a1ddd244752 related to management-hold did help, see 0001-fix-management-hold.patch (patch is against 2.3.7).

Last edited 5 years ago by kristov (previous) (diff)

comment:2 Changed 5 years ago by Gert Döring

Milestone: release 2.3.8
Owner: set to Steffan Karger
Status: newassigned

Thanks for the detailed report and patch. Patch looks reasonable, but throwing to syzzer, who did the original patch ("we had the suspicion that some combinations of options might cause side effects")...

comment:3 Changed 5 years ago by Steffan Karger

Good bugreport indeed. And apologies for breaking your setup.

When looking at this, I realized we can actually further simplify the code *and* fix your bug in the same time. I attached a patch of which I believe does both (based on release/2.3). Could you try it out in your setup?

Changed 5 years ago by Steffan Karger

comment:4 Changed 5 years ago by kristov

Thank you, syzzer, your patch also fixes the problem.

comment:5 Changed 5 years ago by Gert Döring

commit 659eae7b79e5565bb0c93f6d6d04e2163fea1141 (master)
commit bce656d27fe06ed364a4acebd3c3d6d996750613 (release/2.3)

Author: Steffan Karger
Date: Fri Jun 19 00:08:45 2015 +0200

write pid file immediately after daemonizing

committed and pushed (that's the patch from the attachment). Will be part of 2.3.8, but the release date of that is not planned for very near future (unless something really searious comes up).

comment:6 Changed 5 years ago by Gert Döring

Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.