tmp¶

Labels: enhancement, bug, documentation, cleanup, fixed / solved / done

pcahyna opened issue at 2021-08-02 16:39:¶

Pull Request Details:¶

Type: Bug Fix
Impact: Normal
Reference to related issue (URL): https://github.com/rear/rear/issues/2654#issuecomment-881609874
How was this pull request tested?
Running rear mkrescue on a system with /tmp on tmpfs. Before, it has failed with the message cp: error copying '/tmp/rear.ZLTj0NcVqlr4fiH/tmp/initrd.cgz' to '/tmp/rear.ZLTj0NcVqlr4fiH/tmp/isofs/isolinux/initrd.cgz': No space left on device. After the change it succeeds.
Brief description of the changes in this pull request:

The default temporary directory ( /tmp ) is not suited for ReaR, because ReaR needs lots of space. file-hierarchy(7) recommends to use /var/tmp as default for such programs.

If the user sets TMPDIR explicitly, it still takes precedence.

See the discussion in issue #2654 .

hpannenb commented at 2021-08-03 07:24:¶

@pcahyna Would it make sense to prevent overwriting the TMPDIR if set elsewhere already? I therefore would propose a line like this test "$TMPDIR" || export TMPDIR="${TMPDIR-/var/tmp}" in default.conf.

pcahyna commented at 2021-08-03 07:57:¶

@pcahyna Would it make sense to prevent overwriting the TMPDIR if set elsewhere already? I therefore would propose a line like this test "$TMPDIR" || export TMPDIR="${TMPDIR-/var/tmp}" in default.conf.

@hpannenb that's what my line is doing. If TMPDIR is set, it overwrites it with the same value, which should be a no-op (and .there are many other instances of this idiom in the code).
Note that I respect TMPDIR even if it is set to an empty value (maybe I should set it to /var/tmp in this case, too).

hpannenb commented at 2021-08-03 08:11:¶

@pcahyna I see. I am not aware of all those bash "specialties". Thanks for Your explanation.

jsmeix commented at 2021-08-03 08:54:¶

@pcahyna
thank you for this pull request!

Did you test that "rear recover" works with
the new TMPDIR=/var/tmp in default.conf
and also with some predefined TMPDIR=/very/special/path/tmp
for the user 'root' on the original system where "rear mkrescue" runs?

I wonder if removing rescue/GNU/Linux/600_unset_TMPDIR_in_rescue_conf.sh
is always fully safe because the comment in that file indicates that something like
TMPDIR=/var/tmp may not work during "rear recover":

# e.g. by defining TMPDIR=/var we would get our BUILD_DIR=/var/tmp/rear.XXXXXXXXXXXX
# However, in rescue we want our BUILD_DIR=/tmp/rear.XXXXXXX ...

But as far as I see from plain looking at the code
we should have /var/tmp in the recovery system in any case
because the skel/default/var/tmp directory (with its contents)
gets included by rescue/default/010_merge_skeletons.sh
so "rear recover" should work with the TMPDIR=/var/tmp default
and
I think the user 'root' in the recovery system cannot inherit
some TMPDIR=/very/special/path/tmp setting
from the user 'root' on the original system
so "rear recover" should work with our TMPDIR=/var/tmp default even
if there is TMPDIR=/very/special/path/tmp for 'root' on the original system
but I am not 100% sure.

pcahyna commented at 2021-08-03 09:03:¶

Did you test that "rear recover" works with
the new TMPDIR=/var/tmp in default.conf
and also with some predefined TMPDIR=/very/special/path/tmp
for the user 'root' on the original system where "rear mkrescue" runs?

No, I will test this.

e.g. by defining TMPDIR=/var we would get our BUILD_DIR=/var/tmp/rear.XXXXXXXXXXXX

This code comment looks wrong. If you define TMPDIR=/var, then BUILD_DIR should be /var/rear.XXXXXXXXXXXX.

I think the user 'root' in the recovery system cannot inherit
some TMPDIR=/very/special/path/tmp setting
from the user 'root' on the original system
so "rear recover" should work with our TMPDIR=/var/tmp default even
if there is TMPDIR=/very/special/path/tmp for 'root' on the original system
but I am not 100% sure.

You mean if it is set in the root's dotfiles? Not sure whether ReaR can inherit them in some way, but if it does, the former code is broken anyway (the unset is too late).

jsmeix commented at 2021-08-03 09:46:¶

I also think that code comment is a typo. It originated from
https://github.com/rear/rear/commit/179fd6493883ddbba740fd64e4851ab322bc39ca

For me the user 'root' in the recovery system has only

/tmp/rear.XXX/rootfs/root/.ssh
/tmp/rear.XXX/rootfs/root/.ssh/known_hosts
/tmp/rear.XXX/rootfs/root/.ssh/authorized_keys
/tmp/rear.XXX/rootfs/root/.gitignore
/tmp/rear.XXX/rootfs/root/.bash_history

with those contents

# diff -s ~/.ssh/known_hosts /tmp/rear.XXX/rootfs/root/.ssh/known_hosts
Files /root/.ssh/known_hosts and /tmp/rear.XXX/rootfs/root/.ssh/known_hosts are identical

# diff -s ~/.ssh/authorized_keys /tmp/rear.XXX/rootfs/root/.ssh/authorized_keys 
Files /root/.ssh/authorized_keys and /tmp/rear.XXX/rootfs/root/.ssh/authorized_keys are identical

# cat /tmp/rear.XXX/rootfs/root/.bash_history
: # no more predefined ReaR entries in the bash history
systemctl start sshd.service              # start SSH daemon
ip -4 addr                                # get IPv4 address
dhcpcd eth0                               # start DHCP client
nano /var/lib/rear/layout/diskrestore.sh  # disk restore
nano /var/lib/rear/layout/disklayout.conf # disk layout
vi /var/lib/rear/layout/diskrestore.sh    # disk restore
vi /var/lib/rear/layout/disklayout.conf   # disk layout
less /var/log/rear/*                      # log file(s)
loadkeys -d                               # default keyboard
rear recover                              # recover system
: # there are some predefined entries in the bash history

so for me nothing from 'root' on the original system
gets copied into the recovery system except
/root/.ssh/known_hosts and /root/.ssh/authorized_keys
(I have SSH_ROOT_PASSWORD="rear" in my etc/rear/local.conf)
cf. rescue/default/500_ssh.sh

Of course the user could copy anything via COPY_AS_IS
but then he is responsible what he gets in his recovery system.

jsmeix commented at 2021-08-03 09:51:¶

@hpannenb
could you test if things work OK for your use case
with this changes here?

hpannenb commented at 2021-08-03 09:58:¶

@hpannenb
could you test if things work OK for your use case
with this changes here?

I will need to check here on a CentOS7 VM. But I guess it will fit.

Update for @jsmeix : I could successfully clone my Centos7 VM with means of the current ReaR git version plus the changes made by @pcahyna. My test setup is NETFS backup included into bootable ISO as follows:

# ReaR - site.conf

BACKUP=NETFS
BACKUP_URL=iso://backup

BACKUP_PROG_EXCLUDE=( $BACKUP_PROG_EXCLUDE[@] '/var/tmp/*' '/var/backup/*' '/backup/*' )
OUTPUT=ISO
OUTPUT_URL=null

ISO_MAX_SIZE=2100

USE_STATIC_NETWORKING=yes
USE_RESOLV_CONF=no

SSH_ROOT_PASSWORD='root'

P.S.: The /tmp directory is mounted noexec,nodev,nosuid.

jsmeix commented at 2021-08-03 10:55:¶

FYI by the way regarding
"inherited stuff from 'root' on the original system could let things in 'rear recover' fail":

Now I remember where that happened:
It happened during "rear recover" 'finalize' stage (i.e. after the backup was restored)
where certain things are run inside the restored system via chroot /mnt/local
(in particular recreating the initrd and reinstalling the bootloader),
see https://github.com/rear/rear/issues/862
and https://github.com/rear/rear/pull/1345

The unfortunate consequence in current ReaR code is that
we run a COMMAND within chroot /mnt/local differently:

With a login shell
e.g. in finalize/Linux-i386/660_install_grub2.sh

chroot $TARGET_FS_ROOT /bin/bash --login -c "COMMAND"

With a shell that is no login shell
e.g. in finalize/SUSE_LINUX/i386/550_rebuild_initramfs.sh

chroot $TARGET_FS_ROOT /bin/bash -c "COMMAND"

Without a shell
e.g. in finalize/Fedora/i386/550_rebuild_initramfs.sh

chroot $TARGET_FS_ROOT COMMAND

Seems to be something more to clean up in the future...

jsmeix commented at 2021-08-05 07:38:¶

@rear/contributors
I would like to merge it tomorrow morning
unless there are objections.

jsmeix commented at 2021-08-05 07:40:¶

@pcahyna
could you or did you already test that "rear recover"
works with the new TMPDIR=/var/tmp in default.conf
and also with some predefined TMPDIR=/very/special/path/tmp
for the user 'root' on the original system where "rear mkrescue" runs?

hpannenb commented at 2021-08-05 09:53:¶

@pcahyna
could you or did you already test that "rear recover"
works with the new TMPDIR=/var/tmp in default.conf
and also with some predefined TMPDIR=/very/special/path/tmp
for the user 'root' on the original system where "rear mkrescue" runs?

@jsmeix As mentioned in my update a recovery works for one of my use cases. No objections in that area.

pcahyna commented at 2021-08-05 09:59:¶

could you or did you already test that "rear recover"
works with the new TMPDIR=/var/tmp in default.conf
and also with some predefined TMPDIR=/very/special/path/tmp
for the user 'root' on the original system where "rear mkrescue" runs?

@jsmeix As mentioned in my update a recovery works for one of my use cases. No objections in that area.

@hpannenb have you also tested setting a custom TMPDIR in the program environment, or just the default setting?

hpannenb commented at 2021-08-05 10:10:¶

[...]

@hpannenb have you also tested setting a custom TMPDIR in the program environment, or just the default setting?

I am currently testing with export TMPDIR="/home/hpannenb/megarear/".

jsmeix commented at 2021-08-05 10:29:¶

I am currently testing as well...

jsmeix commented at 2021-08-05 11:09:¶

"rear mkbackup" and "rear recover" work well for me both
with the default with no TMPDIR set by root
where /var/tmp/rear.XXX is used both for "rear mkbackup" and "rear recover"
and also
with TMPDIR set by root on the original system as

# mkdir -p /opt/QQQ/rear/tmp
# export TMPDIR="/opt/QQQ/rear/tmp"

where /opt/QQQ/rear/tmp/rear.XXX is used for "rear mkbackup"
and the default /var/tmp/rear.XXX is used for "rear recover"
(i.e. no TMPDIR is set by root in the recovery system).

hpannenb commented at 2021-08-05 13:40:¶

rear mkbackup and rear recover and either default or export TMPDIR="/home/hpannenb/megarear/" work for me as well in my Centos7 based VM.

pcahyna commented at 2021-08-05 13:55:¶

thank you @jsmeix and @hpannenb for those tests and sorry for not having tested it myself.

jsmeix commented at 2021-08-06 05:36:¶

@hpannenb
thank you for also testing it.
It helps a lot to avoid unexpected regressions
when more than one independent tests are done.

@pcahyna
thank you for finally solving this long lasting case
what the right way is to set ReaR's temporary directory
that goes back at least to the year 2013
(as far as I found out by looking for TMPDIR in git log).

jsmeix commented at 2021-08-06 06:10:¶

@pcahyna
I have a question about the exact intention of the command

export TMPDIR="${TMPDIR-/var/tmp}"

where you wrote above in
https://github.com/rear/rear/pull/2664#issuecomment-891624592
"... should be a no-op ..."

According to how I understand "man bash" that command
exports TMPDIR in any case (so it is not a no-op because it exports it)
and the value of TMPDIR is set to /var/tmp only if TMPDIR is unset
but not if TMPDIR is null/empty so if TMPDIR is empty it results

export TMPDIR=""

Is my understanding right?

If yes, I wonder what the intent is to export an empty TMPDIR variable?
Is the export in any case perhaps meant to ensure that any TMPDIR value
(also when it is the empty value which means "use the system default")
will be in the environment for all exec-ed (and possibly fork-ed) programs?
I.e. if somehow TMPDIR was set but not yet exported it is then exported?

If this is the intent behind I think it is good to do it this way
to ensure a set TMPDIR is used by all subsequent programs.

hpannenb commented at 2021-08-06 08:35:¶

@jsmeix As recognised from here and man bash there are many ways to substitute. That is what @pcahyna also mentions in his comment " Note that I respect TMPDIR even if it is set to an empty value (maybe I should set it to /var/tmp in this case, too)."

It might be useful to always overwrite with a default /var/tmp in any case but the user explicitly decides to use a different path:

I did a small check on this (though I am not a bash expert):

[root@centos7 ~]# unset TMPDIR
[root@centos7 ~]# echo "${TMPDIR-/var/tmp}","${TMPDIR:-/var/tmp}","${TMPDIR:=/var/tmp}"
/var/tmp,/var/tmp,/var/tmp
[root@centos7 ~]# TMPDIR=
[root@centos7 ~]# echo "${TMPDIR-/var/tmp}","${TMPDIR:-/var/tmp}","${TMPDIR:=/var/tmp}"
,/var/tmp,/var/tmp
[root@centos7 ~]# TMPDIR=aaaaa
[root@centos7 ~]# echo "${TMPDIR-/var/tmp}","${TMPDIR:-/var/tmp}","${TMPDIR:=/var/tmp}"
aaaaa,aaaaa,aaaaa

A general decision to be made what will be the better approach to use.

jsmeix commented at 2021-08-06 09:03:¶

The "respect TMPDIR even if it is set to an empty value" is my point.

My general "mantra" in particular for ReaR is

Final power to the user!

In this case "final power to the user" means:
When TMPDIR is set we assume it is set by the user so ReaR has to obey
and ReaR must error out if it cannot work with what is set (by its user)
but ReaR must not make a final decision against what is set (by its user).
In contrast when TMPDIR is not set then ReaR is free to decide.

hpannenb commented at 2021-08-06 09:42:¶

Acceptable "red line"/mantra for the use of ReaR. So the solution is kept "as is". 👍

pcahyna commented at 2021-08-19 11:59:¶

@jsmeix sorry for not having noticed your question earlier. Indeed, TMPDIR being empty is a corner case and the code does not touch the value of the variable in this case but merely exports it. But I think that in practice it does not matter, because the variable can't be set without being exported - if it is not exported, how would ReaR have inherited it? The only possibility is that someone has set it in a configuration file, without exporting it, but I don't think we should consider this really supported.
Concerning

the value of TMPDIR is set to /var/tmp only if TMPDIR is unset but not if TMPDIR is null/empty

maybe I should change it to :- to set the variable to a default value both if it is unset or empty. I am not at all sure what is the intended semantics of TMPDIR set to an empty string, if any.

jsmeix commented at 2021-08-31 11:43:¶

@pcahyna
see my https://github.com/rear/rear/pull/2664#issuecomment-894117682

I.e. I think when TMPDIR is set (i.e. also when it is set to an empty value)
we assume it is set intentionally by the user (also if it is set to an empty value
we assume the user wants the semantics of that setting) and ReaR has to obey
so I think it is exactly right to let ReaR set TMPDIR only if TMPDIR is unset.

[Export of Github issue for rear/rear.]

#2664 PR merged: Default TMPDIR should be /var/tmp¶