Opened 5 years ago

Last modified 3 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


Change History (7)

comment:1 Changed 4 years ago by zzz

Owner: changed from icu812 to Masayuki Hatta
Status: newassigned

comment:2 Changed 3 years ago by zzz

Milestone: 0.9.340.9.35

comment:3 Changed 3 years ago by zzz

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.

comment:4 Changed 3 years ago by zzz

Milestone: 0.9.350.9.38

@mhatta would this fix it?

--- debian/i2p.init	ea8bda5cf970d5b3bf38839447ae1e5a542e7f53
+++ debian/i2p.init	218a1a105579ec730a06e914688011c58cc4bf78
@@ -25,7 +25,7 @@ I2P="/usr/share/i2p"
+I2PTEMP=`mktmp -d -p ${NAME}-daemon.XXXXXXXXXX`
 # Don't touch these, edit /etc/default/i2p

comment:5 Changed 3 years ago by Masayuki Hatta

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 3 years ago by zzz

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 ) 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"
 # Don't touch these, edit /etc/default/i2p
@@ -35,9 +34,8 @@ I2P_ARGS="/etc/i2p/wrapper.config \
 I2P_ARGS="/etc/i2p/wrapper.config \ \
-$RUN \
+$RUN \
  wrapper.logfile=$WRAPPERLOG \
  wrapper.pidfile=$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
-    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 3 years ago by zzz

Milestone: 0.9.380.9.39

Milestone set to 39 as requested by mhatta.
I still haven't tested the above patch.
mhatta please test and report results.

Note: See TracTickets for help on using tickets.