Bug#796635: nvi init script still needed?
Hi, So AFAICT this init script: Deletes old recovery/crash files from the nvi text editor Possibly emails some of them to the users This seems like it makes more sense as a cron job that an administrator can customize instead of a script that only runs at bootup. I'd suggest just having it available as a script and add an example cronjob administrators can use.
Bug#796635: nvi init script still needed?
Depending on how it's modified to fix that bug, I think it could introduce a security issue as it: * doesn't seem like an upstream script designed to run as root * seems racy (especially after checking if something is a symlink) * handles user content as root AFAICT being at runlevel S at least stops racy issues. I wasn't actually able to exploit anything myself, but if say there is an exploit in awk, this could allow the attacker to get root at the next reboot.
Bug#796635: Runlevel S to 1
This appears to be the simplest fix for the bug, moving from using runlevel S to runlevel 1. I don't believe this makes the security situation any worse. nvi_1.81.6-12.debdiff Description: Binary data
Bug#796635: Runlevel S to 1
Apologies, that debdiff will not solve our problems and will break badly on sysv.
Bug#796635: Systemd job
Here is a debdiff that implements a systemd unit. (This is the first unit I've written, so review definitely needed) nvi_1.81.6-12.debdiff Description: Binary data
Bug#796635: Systemd job
Thanks for the review Felipe > Why did you preserve runlevel S? I don't think this really belongs in > recovery mode. Changed >> +;; >> + stop|restart|reload|force-reload) > > restart (and force-reload?) should probably re-run the recovery script. Changed. >> +Description=To recover nvi edit sessions. > > s/To r/R/ ? +1 > Also, the init script Provides: nviboot, so you should provide a link > from nviboot.service to nvi.service in /lib/systemd/system. I changed it so it provided nvi. That was the whole reason I changed the name afterall. >> ++if [ -n "$sessions_found" ] ; then >> ++echo "done." >> ++else >> ++echo "none found." >> ++fi > > This is a behavior change: previously the recover script would not > print any output. Maybe this should not be printed? The previous init script did print none found though. > Why do you invoke dh_systemd_enable manually? the --with=systemd above > should do it. > >> + dh_installinit -- start 70 S . > > start argument is obsolete now and just invokes `defaults`, so this > override can go. Because I didn't know what i was doing, oops. > However, this changes the name of the init script from nviboot to nvi. > This means you need to use dpkg-maintscript-helper to handle the > rename, and remove the old enablement links as well. If you want to do > this you might as well move it out of runlevel S ;) Moved out of runlevel S and it deletes nviboot correctly now. Thanks again, Bryan nvi_1.81.6-12.debdiff Description: Binary data
Bug#796635: Systemd job
On Mon, Nov 2, 2015 at 5:24 PM, Felipe Sateler wrote: ++if [ -n "$sessions_found" ] ; then ++echo "done." ++else ++echo "none found." ++fi >>> >>> This is a behavior change: previously the recover script would not >>> print any output. Maybe this should not be printed? >> >> The previous init script did print none found though. > > Maybe I'm worrying too much (I leave it to you to decide), but maybe > people are using this in cronjobs or other scripted environments, and > this changes the output. I don't think there's an obvious win either way. Either it's closer the previous init script or to the previous recover script. > >> >>> Why do you invoke dh_systemd_enable manually? the --with=systemd above >>> should do it. >>> + dh_installinit -- start 70 S . >>> >>> start argument is obsolete now and just invokes `defaults`, so this >>> override can go. >> >> Because I didn't know what i was doing, oops. > > You removed the --no-start --no-restart-on-upgrade arguments to > dh_systemd_start, are you sure you want to restart on package > install/upgrade? Yea, I was thinking about that. Might as well let the script run on package upgrade. It shouldn't hurt anything and it might clear out old files before an upgrade on machines that haven't been restarted recently. >>> However, this changes the name of the init script from nviboot to nvi. >>> This means you need to use dpkg-maintscript-helper to handle the >>> rename, and remove the old enablement links as well. If you want to do >>> this you might as well move it out of runlevel S ;) >> >> Moved out of runlevel S and it deletes nviboot correctly now. > > Cool. Except for 3 things: > > 1. Instead of adding the rm_connfile calls maually, you can use the > dh_installdeb support. Check the manpage for details (basically, put > the rm_conffile part only once in debian/nvi.maintscript). Heh. that's much quicker. Thanks! > > 2. This is not really a rm_conffile, but rather a mv_conffile (the > script is renamed, not removed). Really the init get's split up to nvi and the recover script. If the modified the recover script logic the mv_conffile wouldn't help them. > > 3. This will not preserve enable/disable modifications by the user. > There is no simple solution for this (you need to check in preinst the > state if differing from the default, and restore it in postinst for > the new script name), so maybe just adding a note to NEWS.Debian > suffices? Any modifications made to the init script will be lost > anyways when using systemd as then only the service file will be used. Added to to NEWS.Debian. >Also, you might want to remove the - from the su call. A login shell is not >require Done. > Thanks for your work! Thanks for all your work reviewing this! I learned a bunch. Kind regards, Bryan nvi_1.81.6-12.debdiff Description: Binary data
Bug#796635: Systemd job
> You need to remove the -- "$@" part: it is being added twice to the
> resulting scripts (check debian/nvi/DEBIAN/{pre,post}* after a build):
>
> dpkg-maintscript-helper rm_conffile /etc/init.d/nviboot 1.81.6-12 nvi
> -- "$@" -- "$@"
>
>>> Thanks for your work!
>> Thanks for all your work reviewing this! I learned a bunch.
>
> After the above change I think this would be ready for upload. Can you
> upload or do you need sponsoring? I can do the actual upload.
I need sponsoring.
Thanks!
Bryan
nvi_1.81.6-12.debdiff
Description: Binary data
