Opened 5 years ago
Last modified 2 years ago
#1837 assigned defect
Use less risky code for /tmp handling in I2P initscript and systemd unit file
Reported by: | zzz | Owned by: | Masayuki Hatta |
---|---|---|---|
Priority: | minor | Milestone: | 0.9.39 |
Component: | package/debian | Version: | 0.9.26 |
Keywords: | Cc: | ||
Parent Tickets: | Sensitive: | no |
Description
Subtickets
Change History (7)
comment:1 Changed 3 years ago by
Milestone: | 0.9.27 → 0.9.34 |
---|---|
Owner: | changed from icu812 to Masayuki Hatta |
Status: | new → assigned |
comment:2 Changed 3 years ago by
Milestone: | 0.9.34 → 0.9.35 |
---|
comment:3 Changed 3 years ago by
comment:4 Changed 2 years ago by
Milestone: | 0.9.35 → 0.9.38 |
---|
@mhatta would this fix it?
--- debian/i2p.init ea8bda5cf970d5b3bf38839447ae1e5a542e7f53 +++ debian/i2p.init 218a1a105579ec730a06e914688011c58cc4bf78 @@ -25,7 +25,7 @@ I2P="/usr/share/i2p" PIDFILE="$RUN/$NAME.pid" JVMPIDFILE="$RUN/routerjvm.pid" I2P="/usr/share/i2p" -I2PTEMP="/tmp/${NAME}-daemon" +I2PTEMP=`mktmp -d -p ${NAME}-daemon.XXXXXXXXXX` WRAPPERLOG="/var/log/i2p/wrapper.log" # Don't touch these, edit /etc/default/i2p
comment:5 Changed 2 years ago by
I'm not sure why I2PTEMP is specified separately (can it simply use the same /tmp/i2p-XXXXXXXX.tmp?), but anyway the use of mktemp should be a good thing.
comment:6 Changed 2 years ago by
oops. several problems with the patch in comment 4, which I obviously didn't test:
- it's mktemp, not mktmp
- it must be —tmpdir not -p (the last arg is the template, not the arg for -p)
- the whole thing won't work, we can't make a new directory every time you run the init script (for example when the arg is stop or status)
So that won't work at all.
Comment 3 above has the main issues. The i2p.dir.temp system property is not i2p's temp dir, but its PARENT. I2PAppContext creates a unique temp dir inside it. The only reason for i2p.init to create, specify, and delete a temp dir is if it must put its own files in there.
A review of the checkin history shows that it did used to put its own files there, but does not now. Back in 2011, kytv did have a "anchor file" in there (see https://wrapper.tanukisoftware.com/doc/english/prop-anchorfile.html ) but deleted it within a couple weeks, as it wasn't being used. Similarly, the pid files used to be in $I2PTEMP, but in late 2011 he moved them to /var/run/i2p .
So an inspection of i2p.init today reveals no files placed inside there by i2p.init. The sole content is the directory created there by I2PAppContext. Therefore, we can remove all $I2PTEMP references in i2p.init. Additionally, The wrapper additional params should be consecutive (not 1 10 11 12).
So that leads me to this patch, which I haven't tested either:
# # old_revision [effa099b40ff593227c59707a33922176dc775b1] # # patch "debian/i2p.init" # from [ea8bda5cf970d5b3bf38839447ae1e5a542e7f53] # to [0294d721997794ea80e5aee7d55001056fe70a1f] # ============================================================ --- debian/i2p.init ea8bda5cf970d5b3bf38839447ae1e5a542e7f53 +++ debian/i2p.init 0294d721997794ea80e5aee7d55001056fe70a1f @@ -25,7 +25,6 @@ I2P="/usr/share/i2p" PIDFILE="$RUN/$NAME.pid" JVMPIDFILE="$RUN/routerjvm.pid" I2P="/usr/share/i2p" -I2PTEMP="/tmp/${NAME}-daemon" WRAPPERLOG="/var/log/i2p/wrapper.log" # Don't touch these, edit /etc/default/i2p @@ -35,9 +34,8 @@ I2P_ARGS="/etc/i2p/wrapper.config \ I2P_ARGS="/etc/i2p/wrapper.config \ wrapper.java.additional.1=-DloggerFilenameOverride=/var/log/i2p/log-router-@.txt \ - wrapper.java.additional.10=-Dwrapper.logfile=$WRAPPERLOG \ - wrapper.java.additional.11=-Di2p.dir.pid=$RUN \ - wrapper.java.additional.12=-Di2p.dir.temp=$I2PTEMP \ + wrapper.java.additional.2=-Dwrapper.logfile=$WRAPPERLOG \ + wrapper.java.additional.3=-Di2p.dir.pid=$RUN \ wrapper.logfile=$WRAPPERLOG \ wrapper.pidfile=$PIDFILE \ wrapper.java.pidfile=$JVMPIDFILE \ @@ -95,7 +93,6 @@ do_start() start-stop-daemon --start --quiet --pidfile $PIDFILE --exec $DAEMON --test > /dev/null 2>&1 \ || return 1 [ -d $RUN ] || mkdir $RUN > /dev/null 2>&1 - [ -d $I2PTEMP ] || mkdir $I2PTEMP > /dev/null 2>&1 if [ -r $PIDFILE ]; then PID="$(cat ${PIDFILE})" if ! kill -0 $PID > /dev/null 2>&1; then @@ -106,7 +103,7 @@ do_start() return 1 fi fi - chown -Rf $I2PUSER:$I2PUSER $I2PTEMP $RUN > /dev/null 2>&1 + chown -Rf $I2PUSER:$I2PUSER $RUN > /dev/null 2>&1 chown -f -R $I2PUSER:i2psvc /var/log/$NAME > /dev/null 2>&1 if [ "$USE_AA" = "yes" ] && \ [ -x /usr/sbin/aa-status ] && \ @@ -141,7 +138,6 @@ do_stop() start-stop-daemon --stop --quiet --oknodo --retry=0/60/KILL/20 --exec $DAEMON - rm -rf "$I2PTEMP" > /dev/null 2>&1 [ -d "$RUN" ] && rm -f "$RUN/*" > /dev/null 2>&1 [ -d "$RUN" ] && rmdir "$RUN" > /dev/null 2>&1 }
comment:7 Changed 2 years ago by
Milestone: | 0.9.38 → 0.9.39 |
---|
Milestone set to 39 as requested by mhatta.
I still haven't tested the above patch.
mhatta please test and report results.
I'm not sure why we specify the temp dir at all. It defaults to /tmp, and I2PAppContext creates a 700 dir i2p-XXXXXXXX.tmp under that. All users of the temp dir inside i2p code should use I2PAppContext.getTempDir(), which for non-service instances is /tmp/i2p-XXXXXXXX.tmp/, and for the service instances, is /tmp/i2p-daemon/i2p-XXXXXXXX.tmp/ because of the override in the init scripts.
If any files were placed directly in /tmp/i2p-daemon/ by our code, that would be more of a problem.
We'll need to research the mtn history for why there's a temp dir specified in the systemd and non-systemd scripts, but it seems like the easiest fix would be to remove it, and bring everything "up" a level.