#3163 PR `merged`: Error out if TMPDIR is set in user config¶

Labels: enhancement, fixed / solved / done

pcahyna opened issue at 2024-02-23 11:01:¶

Relax-and-Recover (ReaR) Pull Request Template¶

Pull Request Details:¶

Type: Enhancement
Impact: Low
Reference to related issue (URL): #2654
How was this pull request tested?
export TMPDIR=/tmp in etc/rear/local.conf and calling rear help (the error appears even with help).
Description of the changes in this pull request:
Error out early if TMPDIR is set in user config - setting TMPDIR in {site,local}.conf has not worked since 0022063 (PR #2633), as discussed in #2654.

pcahyna commented at 2024-02-23 12:29:¶

@rear/contributors please check if you agree with this change. I believe that it is better to abort early than to behave in unexpected ways.

jsmeix commented at 2024-02-23 13:11:¶

@pcahyna
thank you for this improvement.

I fully agree that in particular for "rear mkrescue/mkbackup"
(i.e. for all workflows that run on the original system)
it is always better to abort early than to (silently)
behave in unexpected ways.

For "rear recover" it is different.
Here it is in general better to abort early
than to (silently) behave in unexpected ways.
But for "rear recover" there are certain cases
(e.g. recreation of the initrd)
where it is better to proceed "bona fide" and
try out of something will actually fail later
than to check things in advance and abort in advance
without ever trying the real thing.

jsmeix commented at 2024-02-23 13:14:¶

@pcahyna
I do not understand why you set

saved_tmpdir="${TMPDIR-}"

two times but you do not use the saved_tmpdir value
between when it is set first and when it is set for the second time.
So I do not understand why you set it for the first time.
In particular I fail to understand what you like to tell with

# Can legitimately change in internal defaults, so we will set it again
# after reading them.

pcahyna commented at 2024-02-23 13:24:¶

For "rear recover" it is different.
Here it is in general better to abort early
than to (silently) behave in unexpected ways.
But for "rear recover" there are certain cases
(e.g. recreation of the initrd)
where it is better to proceed "bona fide"

Good point, but I think that this case falls in the first category: even with "recover", we should abort early and let user fix their local.conf, if needed. "recover" can behave in strange ways when TMPDIR is not set properly.

pcahyna commented at 2024-02-23 13:26:¶

I do not understand why you set
saved_tmpdir="${TMPDIR-}"
two times but you do not use the saved_tmpdir value
between when it is set first and when it is set for the second time.

The first initialization can be deleted, I just did not want to leave the value unset, but there was no good reason for it.

jsmeix commented at 2024-02-23 14:06:¶

I think this case falls indirectly in the first category
so it is OK to always error out here.

Details:

I think this case does not directly fall in the first category
because I think "rear recover" likely works
even with TMPDIR (uselessly) set in a user config file
so it could be better for the user to not bother him
and proceed "bona fide" and try out if something
will actually fail during "rear recover".

Nevertheless I think in this case it is OK to always error out
because "rear recover" cannot run without "rear mkrescue"
(or "rear mkbackup") before.
Because the user config files in the ReaR recovery system
are usually same as what they were when "rear mkrescue"
(or "rear mkbackup") was run before, we can safely assume
that "rear recover" will normally not error out with TMPDIR
set in a user config file.

In the unusual case when the user changed a config file
in the ReaR recovery system before running "rear recover"
it is OK to error out when the user had falsely set TMPDIR in
a config file because this will correctly tell the user that
his config file change in the ReaR recovery system was wrong
and how to properly achieve what he intends.

pcahyna commented at 2024-02-23 14:22:¶

I think this case does not directly fall in the first category
because I think "rear recover" likely works
even with TMPDIR (uselessly) set in a user config file
so it could be better for the user to not bother him
and proceed "bona fide" and try out if something
will actually fail during "rear recover"

It may work now, but as soon as some code uses mktemp after the configuration is sourced, it will misbehave with mktemp: failed to create file via template '/foo/bar/tmp.XXXXXXXXXX': No such file or directory if TMPDIR=/foo/bar exists in the original system, but does not exist in the recovery ramdisk.

No such code exists now, but I guess it will appear sooner or later.

EDIT: N.B. this was not an issue until my commit 2721c2743ea959631be0d30eac92984a708c1e66, because there was code to unset TMPDIR in the recovery system.

EDIT: I agree with the rest of what you wrote.

jsmeix commented at 2024-02-26 07:13:¶

Meanwhile I think the

Error "Setting TMPDIR in a configuration file is not supported. To specify a working area directory prefix, export TMPDIR before executing '$PROGRAM'"

is wrong - both the message and the Error() abort.

I think this should be only a user notification like

LogPrintError "Setting TMPDIR in a configuration file does not work to specify ReaR's working area, see default.conf about TMPDIR"

Reason:

Setting TMPDIR in a ReaR config file does not work
to specify ReaR's working area BUILD_DIR
BUT
setting or unsetting TMPDIR in a ReaR config file
is probably the only way to specify a different TMPDIR
(i.e. different than TMPDIR for ReaR's working area)
for the programs that are called by ReaR.

For example assume a user needs to have
ReaR's BUILD_DIR in /path/to/much/space
but he wants to keep the system's default TMPDIR
for the programs that are called by ReaR.

pcahyna commented at 2024-02-26 17:27:¶

@jsmeix

... wrong - both the message and the Error() abort.
I think this should be only a user notification ...

I could do this, but the motivation for aborting early is to prevent nasty surprises when recovering, since while one takes care to ensure that the TMPDIR exists in the original system, I am afraid that essentially nobody realizes the implication that the same TMPDIR would be used for recovery as well and thus needs to be included in the ramdisk. This thus leads to frustration when the recovery is actually needed (often under time pressure) and resulting support calls (that dedicated support or I will have to handle, or will land in our issue tracker). The case when one sets TMPDIR back to the default is safe, but most other cases will not be. Not supporting it is thus the simplest, most robust solution.

If we really decide to downgrade it to printing and continuing despite what I wrote above, then I perhaps should revert 2721c2743ea959631be0d30eac92984a708c1e66, since the motivation for that was that setting TMPDIR in the middle of user configuration is unsupported anyway, so the code is unneeded. Restoring it would help with the concerns above.

Note though that supporting the case where one needs to have ReaR's BUILD_DIR in /path/to/much/space but one wants to keep the system's default TMPDIR for the programs that are called by ReaR is risky. What if for some reason we move the creation of the build area below sourcing of the user config again? Then it would break, as it would create BUILD_DIR in the system default location instead of in /path/to/much/space. I don't think we should teach users to rely on such intimate details of the implementation. Are you aware of any case where an user wants TMPDIR for called programs to be different than for the build area, anyway?

jsmeix commented at 2024-02-27 06:42:¶

By the way
regarding where TMPDIR= is used in ReaR code:

# find usr/sbin/rear usr/share/rear/ -type f | xargs grep 'TMPDIR' | grep -v ':[ ]*#'

usr/share/rear/conf/examples/rescue-and-backup-on-same-ISO-image-example.conf:
TMPDIR=/mnt2/tmp

usr/share/rear/conf/default.conf:
export TMPDIR="${TMPDIR-/var/tmp}"

usr/share/rear/restore/DUPLICITY/default/200_prompt_user_to_start_restore.sh:
test $TMPDIR && old_TMPDIR=$TMPDIR
export TMPDIR=$TARGET_FS_ROOT
test $old_TMPDIR && TMPDIR=$old_TMPDIR || unset TMPDIR

usr/share/rear/output/USB/Linux-i386/100_create_efiboot.sh:
efi_mpt=$( mktemp -d $TMPDIR/rear-efi.XXXXXXXXXX ) || Error "mktemp failed to create mount point '$TMPDIR/rear-efi.XXXXXXXXXX' for EFI partition '$efi_part'"

So there is a wrong config example TMPDIR=/mnt2/tmp in
usr/share/rear/conf/examples/rescue-and-backup-on-same-ISO-image-example.conf

# The defaul location /tmp might be too small to contain the backup (temporary location)
# use an alternative for /tmp (as backup.tar.gz size might be too big for /tmp)
TMPDIR=/mnt2/tmp

jsmeix commented at 2024-02-27 06:44:¶

Via
https://github.com/rear/rear/commit/9793364ba85aac019f367f45f906e55e2e4d4648
I removed in
conf/examples/rescue-and-backup-on-same-ISO-image-example.conf
the wrong and outdated TMPDIR=/mnt2/tmp
that is no longer needed since the default
is /var/tmp which has enough space.

jsmeix commented at 2024-02-27 07:06:¶

@pcahyna
thank you so much for your endless work and patience
with that endless "how to properly deal with TMPDIR" stuff!

This makes THE difference between quick hacks
versus thorougly thought-out practicable solutions
even if a solution cannot solve all possible cases
but we can only do what is possible with reasonable effort.

Please merge it as soon as you can.

pcahyna commented at 2024-02-27 10:36:¶

@jsmeix thank you for the thoughtful review and correction of the obsolete example! Merging.
By the way:
you searched for TMPDIR, but any invocation of mktemp implicitely uses TMPDIR, so we have some more uses beyond those that you found.

jsmeix commented at 2024-02-27 10:50:¶

@pcahyna
I searched for TMPDIR because I was interested
what we do with the TMPDIR variable itself
in particular where we modify it
(not where we use its value e.g. via mktemp).

But I missed to search our documentation:

# find doc -type f | xargs grep 'TMPDIR'

doc/user-guide/03-configuration.adoc:
TMPDIR=/bigdisk::
The +TMPDIR+ is picked up by the +mktemp+ command to create the +BUILD_DIR+ under +/bigdisk/tmp/rear.XXXX+
The default value of +TMPDIR+ is an empty string, therefore, by default +BUILD_DIR+ is +/tmp/rear.XXXX+

I will also fix that right now...

jsmeix commented at 2024-02-27 10:58:¶

Via
https://github.com/rear/rear/commit/d5556a3a0a481e941a4604b137978ca0d2d6dcc6
I removed in doc/user-guide/03-configuration.adoc
the outdated and obsolete part about TMPDIR=/bigdisk
because since the default is /var/tmp
there is enough space for things like a big ISO image

pcahyna commented at 2024-02-27 11:05:¶

@jsmeix good catch! I think that documenting default values separately from default.conf is often a bad idea - the documentation becomes soon obsolete.

[Export of Github issue for rear/rear.]

#3163 PR merged: Error out if TMPDIR is set in user config¶