Hi,
Bryan Steele wrote on Sun, Jul 21, 2019 at 01:53:49PM -0400:
> On Sat, Jul 20, 2019 at 12:03:03AM +0200, Jesper Wallin wrote:
>> Oh, you're right. A bit ironic that I didn't notice the exec violation
>> due to the fork being permitted now. Thanks for pointing this out!
>> Scrap my old patch, here's a better proposal:
[ ... and later, on a different patch ]
> I somehow missed that. Indeed, moving the check into rcv_email before
> fork makes more sense. Sorry for jumping the gun.
Some observations, in decreasing order of importance:
1. Jesper, thanks for finding and reporting the regression.
It is fixed now.
2. Bryan, thanks for suggesting the direction how to fix it.
3. Jesper, including a patch according to the best of your
understanding is always welcome. Even if it turns out to be a
bad patch, because often even a bad patch helps to understand
what the OP thinks the problem might be: the saying is, bad
patches trigger good ones.
4. Merely because something different eventually gets committed
does not necessarily mean the original idea was bad. For many
problems, there is more than one solution, and it is often a
matter of judgement or taste which one is better.
Jesper, in this case, re-adding "proc exec" would indeed have
been an alternative solution - even though preventing "exec"
is kind of the whole point of the -S option.
5. It might seem obvious, but after writing a patch and before
sending it out, testing it makes sense. The easy part is testing
that it actually solves the original problem. The slightly
harder part is testing that is also solves similar problems (in
this case, when catching SIGTERM). The hardest part is testing
that it causes no regressions.
6. The history of this thread (the original commit of rev. 1.29,
the initial suggestion to re-add "proc" only, the suggestion
to skip rcv_mailfile()) proves once again that testing is
surprisingly hard in practice. Most definitely, i also
botched testing for many of the commits that i did in the
past, and i also got many OKs for broken patches that i
posted prematurely. It happens, even when trying to be
careful.
7. Rarely used options rarely get tested. They attracks bugs like
rotting fruits attract flies. So think twice before adding a
new option to a program.
Yours,
Ingo
CVSROOT: /cvs
Module name: src
Changes by: schwa...@cvs.openbsd.org 2019/07/22 06:39:02
Modified files:
usr.bin/vi/common: recover.c
Log message:
In secure mode (-S), skip sending mail when executing the :pre[serve]
command or when dying from SIGTERM. This way, creating the recovery
file works again without re-adding "proc exec" to the pledge(2).
As reported by Jesper Wallin <jesper at ifconfig dot se>, this got
broken by common/main.c rev. 1.29 (Nov 19, 2015).
The general direction of the fix was suggested by brynet@.
OK brynet@ and no opposition when shown on tech@
