From owner-svn-src-all@freebsd.org Fri Jun 21 19:32:52 2019 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id B0E3A15BCAE9; Fri, 21 Jun 2019 19:32:52 +0000 (UTC) (envelope-from delphij@gmail.com) Received: from mail-io1-xd44.google.com (mail-io1-xd44.google.com [IPv6:2607:f8b0:4864:20::d44]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 48456754ED; Fri, 21 Jun 2019 19:32:52 +0000 (UTC) (envelope-from delphij@gmail.com) Received: by mail-io1-xd44.google.com with SMTP id n5so376222ioc.7; Fri, 21 Jun 2019 12:32:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=QXmRuwdoRev9wOS+4vKfDJsk9IId6w8cijD06Sm+p00=; b=mhdHsc1G4RD0+LZ4ILx98bxCJ+fWvtNpRksN0vm9/IJBVGyEhGA1voVAfTuiY6WBUK VL2IHP5kSD+CMq9SDPDqmQwe8z9i24+FzupG7umkQoWFWuiXi0Da1ZjJDZX8PVo66vGK 4+8cA8OQzwrvTghI7DcHkxcUcJZHyIN1GrrgooJ//kBrK4pTQJJG0dfu5beOENAY0hsY Bz7uTNT9YC83H6rR95nFJN6M0Tuu5AvOoFO2JTuRKaTg/PL0kx+vBMKtqJNo5Cq/xgpc PlJZd15HdqzJIDmCjZVeuTaBXgwSKvzcNhB29Cn7gjj0a2WEjHa4YXSJgAaism0gDw/W SCpQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=QXmRuwdoRev9wOS+4vKfDJsk9IId6w8cijD06Sm+p00=; b=P+rkrupDWr7DrvSih+SqCB0w2aPQpThJGKEy95XKKd1vvZCG59dYfQCYOhl2JU95xa 4LFknBG2bzhK3WAjs6dTewdiNt0yf0rCxY9TpFaa0CimAn9xt/rPRaqBdVWUt+n6vM0m TMUsnMc9G0XTIcZJcLAqmgMmGZEwcxMYWuvQErnWWmtv3nGCbiWxXUzIkmrhpghdEnQl uqYDY63tcnVyuksOTiJNHa4v2IcIOpMz/f68RXd/baD1M43x9DQyZ8s3vjxeI4JDKJFf J054vr3wVe2g/iJFXuHeyywAoG9z07k09+CBPte9l/YXspGlaBrIZEznzu5oOnOq0iXH 77lA== X-Gm-Message-State: APjAAAV0h/EuOeC/BFXqndFgDGQQ21qm0npfueICRKmZh2PqwlJxU11C u1wZXeRdBqo/RbMwqubMA43ROCbVBU2wfMLXm6X/kDcy X-Google-Smtp-Source: APXvYqyKxFh8f0EMTjtj/sr4qlTqjJ13gwTng197LEHAtKxYS6g2TFa8HATf9FXRvK/FyLIDQm0h7f2QiawoSoi/GFI= X-Received: by 2002:a6b:b987:: with SMTP id j129mr48155240iof.166.1561145571063; Fri, 21 Jun 2019 12:32:51 -0700 (PDT) MIME-Version: 1.0 References: <201906210237.x5L2bt8I012721@repo.freebsd.org> In-Reply-To: <201906210237.x5L2bt8I012721@repo.freebsd.org> From: Xin LI Date: Fri, 21 Jun 2019 12:32:39 -0700 Message-ID: Subject: Re: svn commit: r349256 - head/libexec/rc/rc.d To: Conrad Meyer Cc: "src-committers@freebsd.org" , "svn-src-all@freebsd.org" , "svn-src-head@freebsd.org" X-Rspamd-Queue-Id: 48456754ED X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.97 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-0.998,0]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; REPLY(-4.00)[]; NEURAL_HAM_SHORT(-0.97)[-0.970,0] Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 21 Jun 2019 19:32:52 -0000 On Thu, Jun 20, 2019 at 7:38 PM Conrad Meyer wrote: > Author: cem > Date: Fri Jun 21 02:37:54 2019 > New Revision: 349256 > URL: https://svnweb.freebsd.org/changeset/base/349256 > > Log: > rc.d/motd: Update motd more robustly > > Use appropriate fsyncs to persist the rewritten /etc/motd file, when a > rewrite is performed. > Why is /etc/motd so important to deserve this kind of construct? The worst that could happen to /etc/motd with previous code is that you might end up with an empty or non-existent /etc/motd, and the change have introduced more problems than it intended to solve. > > Reported by: Jonathan Walton > Reviewed by: allanjude, vangyzen > Sponsored by: Dell EMC Isilon > Differential Revision: https://reviews.freebsd.org/D20701 > > Modified: > head/libexec/rc/rc.d/motd > > Modified: head/libexec/rc/rc.d/motd > > ============================================================================== > --- head/libexec/rc/rc.d/motd Fri Jun 21 00:52:30 2019 (r349255) > +++ head/libexec/rc/rc.d/motd Fri Jun 21 02:37:54 2019 (r349256) > @@ -37,11 +37,15 @@ motd_start() > uname -v | sed -e 's,^\([^#]*\) #\(.* > [1-2][0-9][0-9][0-9]\).*/\([^\]*\) $,\1 (\3) #\2,' > ${T} > awk '{if (NR == 1) {if ($1 == "FreeBSD") {next} else {print > "\n"$0}} else {print}}' < /etc/motd >> ${T} > > - cmp -s $T /etc/motd || { > - cp $T /etc/motd > + if ! cmp -s $T /etc/motd; then > + mv -f $T /etc/.motd.tmp > This have partially defeated the benefit of doing mktemp in the code above. The old code tries to avoid excessive writes to /, which is preserved, but it is not safe to assume /etc/.motd.tmp is a non-existent plain file: mv would happily proceed when .motd.tmp is a preexisting directory, for instance (and subsequent mv -f would fail). A malicious system administrator can now plant a timing bomb which will fill / eventually. You can do a TMPDIR=/etc/ mktemp .motd.tmp.XXXXXXXX and use that to mitigate the attack above, but then if the system crash in the middle you would still end up with a dangling .motd.tmp.XXXXXXXX file in /etc. Also note that $T is on /tmp which is likely to be a different filesystem, internally this is mostly equivalent to cp + rm. You can simplify the code by reverting to previous cp and keep the rm -f below as an unconditional remove like before. + fsync /etc/.motd.tmp > + mv -f /etc/.motd.tmp /etc/motd > chmod ${PERMS} /etc/motd > - } > - rm -f $T > + fsync /etc > There is absolutely no reason to fsync /etc here. mv'ing on the same file system is performed with rename(2) which is required to be atomic by POSIX. /etc/motd will be pointing to either the old one or the new one in the event of a crash, and both are acceptable. So, in my opinion the new code would have made the situation worse than the old code. But ultimately, I think the real design question here that needs to be solved would probably be "Why are piling up multiple layers of workarounds around motd? Does it even need to be located in /etc?" The contents is meant to be updated every time when there is a kernel change, and to that extent it seems to be more appropriate for /var/run and generated at boot from a template located somewhere in /etc. The benefit of this approach is that you would have one less file to merge for each etcupdate/mergemaster (or at least only need to do it when some customization is made), and there is no need to worry about write durability.