proftpd service failed to restart

Bug #1246245 reported by Goldhead on 2013-10-30
190
This bug affects 43 people
Affects Status Importance Assigned to Milestone
proftpd-dfsg (Ubuntu)
Undecided
Unassigned

Bug Description

proftpd-basic 1.3.5~rc3-2 from Ubuntu 13.10
proftpd-basic 1.3.5~rc3-2.1ubuntu2 from Ubuntu 14.04

Init script from proftpd-basic package contains the BUG: when you run /etc/init.d/proftpd restart it fails because of there is the race between pidfile removal and start() which checks pidfile existency:

---
ProFTPD is started in standalone mode, currently running.
root@aa:~# /etc/init.d/proftpd restart
 * Stopping ftp server proftpd [ OK ]
 * Starting ftp server proftpd [ OK ]
root@aa:~# /etc/init.d/proftpd status
ProFTPD is started in standalone mode, currently not running.
---

the next workaround helps:

---
--- /etc/init.d/proftpd.orig 2013-10-30 13:52:46.791265726 +0400
+++ /etc/init.d/proftpd 2013-10-30 13:52:57.456265698 +0400
@@ -107,6 +107,7 @@
     fi
     if [ -f "$PIDFILE" ]; then
        start-stop-daemon --stop --signal $SIGNAL --quiet --pidfile "$PIDFILE"
+ sleep 1
         if [ $? = 0 ]; then
                log_end_msg 0
        else
---

Please, fix.

Launchpad Janitor (janitor) wrote :

Status changed to 'Confirmed' because the bug affects multiple users.

Changed in proftpd-dfsg (Ubuntu):
status: New → Confirmed

Thanks Goldhead.
Your patch works perfectly.

Merlijn Hofstra (mhofstra) wrote :

This bug is also still present in 14.04

I tend to think that "sleep 1" is somewhat ugly in the init script, this solution works for me:

@@ -106,7 +106,7 @@
  fi
     fi
     if [ -f "$PIDFILE" ]; then
- start-stop-daemon --stop --signal $SIGNAL --quiet --pidfile "$PIDFILE"
+ start-stop-daemon --stop --signal $SIGNAL --retry 1 --quiet --pidfile "$PIDFILE"
      if [ $? = 0 ]; then
          log_end_msg 0
      else

Thanks Merlijn, your patch works. IMHO your patch is a good way to really fix the race condition.

On Wed, Mar 19, 2014 at 02:11:36AM -0000, joelparkerhenderson wrote:
> Thanks Merlijn, your patch works. IMHO your patch is a good way to
> really fix the race condition.
>

Sorry, not. Adding sleeping seconds is the wrong way of fixing initscripts.
You should instead use the provided interface in start-stop-daemon via --retry.

--
Francesco P. Lovergine

Merlijn Hofstra (mhofstra) wrote :

Hello Francesco,

That's why I submitted a revised patch that does use --retry rather than a sleep. Could you please merge this into the release for 14.04 as this bug is causing me some headaches? :-)

PRISMA Computer GmbH (j-infr-9) wrote :

This solution worked also for me (except I had to edit start-stop-daemon line by hand, I had problems with the diff file itself). But it is not merged in package until 2014-04-07.

Very sad. This bug leads to logrotate is killing proftpd.

description: updated

On Tue, May 06, 2014 at 12:00:46PM -0000, Alexander Birkner wrote:
> - fi
> - if [ -f "$PIDFILE" ]; then
> - start-stop-daemon --stop --signal $SIGNAL --quiet --pidfile "$PIDFILE"
> +      fi
> +      if [ -f "$PIDFILE" ]; then
> +         start-stop-daemon --stop --signal $SIGNAL --quiet --pidfile "$PIDFILE"
> + sleep 1

Unfortunately this is non-solution because timing depends on current load. It
just mitigates the issue, not solves it. And it is a non sense with systemd,
which probably is able to manage better the whole process

> - if [ $? = 0 ]; then
> - log_end_msg 0
> - else
> +          if [ $? = 0 ]; then
> +                 log_end_msg 0
> +         else
> ---
>
> Please, fix.
>

--
Francesco P. Lovergine

Joel (joel-forsberg) wrote :

I just had logrotate trigger this bug on 14.04 (since it supposedly cannot just reload)

Added a re-worked patch, please review.

tags: added: patch
Joel (joel-forsberg) wrote :

I had a mistake, new patch.

The attachment "proftp.patch" seems to be a patch. If it isn't, please remove the "patch" flag from the attachment, remove the "patch" tag, and if you are a member of the ~ubuntu-reviewers, unsubscribe the team.

[This is an automated message performed by a Launchpad user owned by ~brian-murray, for any issues please contact him.]

Joel (joel-forsberg) wrote :

I should probably read up on how to make a patch to the right codebase.

tags: removed: patch
Joel (joel-forsberg) wrote :

Upstream already had changed the init script, but I don't understand it fully.

(looking at master, commit b63ec2128f324610404efa991456b6206095531d File: debian/proftp-basic.init )
My questions:
1) Why exit if $2 is 0? (row 118)
2) Is it necessary to call start-stop-daemon twice if the first it exits with nonzero return value? (row 111 and 115)
3) row 123 looks wrong, attaching simple patch.
4) what is missing to close this ticket and make a corrected .deb? I'm willing to help but know little.

danb1974 (danb1974) wrote :

Seems it's not that simple to do a proper fix, since the same signal() function is used for stop (TERM) and reload (HUP)

If you just add --retry to the first call to start-stop-daemon you will break reload, since proftpd will be killed (--retry changes the behaviour, now it's mission is to terminate the process, sending a TERM/KILL schedule until pid file is gone)

Looks like the clean approach would be to separate stop and reload and get rid of all the if/then/else dance and cascaded start-stop-daemon calls

danb1974 (danb1974) wrote :

My first attempt to cleanup the stop/reload mess, by creating a stop() function which has --retry schedule and leave a simple and clean signal() function. Feel free to point out if I'm totally wrong.

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Duplicates of this bug

Other bug subscribers