Setting BORG_CACHE_DIR to an empty string crashes borg

[lelutin]

Hello,

I've just hit something strange (from the user's point of view) when adding functionality for a helper-script to using borg within backupninja. The feature I was adding was the possibility of displacing the cache dir. For this I set the BORG_CACHE_DIR env var to what users set in the configuration file.

Now the issue that happened was that if somehow the variable BORG_CACHE_DIR ends up empty (and exported as such to the environment), borg uses this empty string as the path to the cache dir and obviously fails, which is confusing for users:

$ export BORG_CACHE_DIR=
$ borg create ./testborg::blah somesourcedir 
Local Exception
Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/borg/archiver.py", line 4529, in main
    exit_code = archiver.run(args)
  File "/usr/lib/python3/dist-packages/borg/archiver.py", line 4461, in run
    return set_ec(func(args))
  File "/usr/lib/python3/dist-packages/borg/archiver.py", line 157, in wrapper
    assert_secure(repository, kwargs['manifest'], self.lock_wait)
  File "/usr/lib/python3/dist-packages/borg/cache.py", line 205, in assert_secure
    sm = SecurityManager(repository)
  File "/usr/lib/python3/dist-packages/borg/cache.py", line 63, in __init__
    self.cache_dir = cache_dir(repository)
  File "/usr/lib/python3/dist-packages/borg/cache.py", line 226, in cache_dir
    return path or os.path.join(get_cache_dir(), repository.id_str)
  File "/usr/lib/python3/dist-packages/borg/helpers.py", line 553, in get_cache_dir
    os.makedirs(cache_dir)
  File "/usr/lib/python3.8/os.py", line 223, in makedirs
    mkdir(name, mode)
FileNotFoundError: [Errno 2] No such file or directory: ''

Platform: Linux meevyl 5.6.0-1-amd64 #1 SMP Debian 5.6.7-1 (2020-04-29) x86_64
Linux: Unknown Linux  
Borg: 1.1.11  Python: CPython 3.8.3 msgpack: 0.5.6
PID: 990872  CWD: /home/gabster/dev
sys.argv: ['/usr/bin/borg', 'create', './testborg::blah', 'backupninja']
SSH_ORIGINAL_COMMAND: None

If the env var BORG_CACHE_DIR exists, I would argue that borg should probably add a check to make sure that it is non-empty. if it is, then it should be discarded and the default cache dir placement should be used.

Bounty: https://www.bountysource.com/issues/91660210-setting-borg_cache_dir-to-an-empty-string-crashes-borg

[ThomasWaldmann]

Putting an empty string there is just a special case of having any invalid string there.

Guess we could easily detect empty, but detecting anything invalid is likely not possible except by just trying to use it. And the latter is exactly what the code does.

And when it blows up trying to use it, guess you can derive that your setting was invalid, right?

[Gu1nness]

One solution would be to test if the BORG_CACHE_DIR exists using the os.path.isdir(path) from the python os stdlib? And if it fails return a message explaining the error?

EDIT : if you're ok with this solution, I'm ready to implement this.

[lelutin]

@Gu1nness that would sound reasonable to me and would be more informative to users (but I'm just a user of this program so maybe there's some factors I did not take into account.

Is borg expected to create the directory if it does not exist? maybe in that case it would be breaking up that assumption?

[Gu1nness]

Indeed, after a look at the code if the path does not exists, it creates the cache dir.
The other way around is to catch the exceptions raised here by os.makedirs and raise a message according to the exception (E.G: permission denied, file not found, etc.)

Other (longer) solution : https://stackoverflow.com/questions/9532499/check-whether-a-path-is-valid-in-python-without-creating-a-file-at-the-paths-ta

[ThomasWaldmann]

When transforming the not-so-pretty exception to a prettier error message, always make sure to also include the exception message so no information is lost.

[ThomasWaldmann]

@Gu1nness I had a look at the code you pointed to and there are quite some places where borg does:

"if not path exists: makedirs + chmod"

Guess this could be refactored into a function like:

ensure_dir(path, mode=None, reraise=False)

That would:

• make sure the directory exists, in a race-condition free way (thus: not using os.path.exists + os.makedirs)
• if mode is not None, do a chmod to that mode if we created the directory right before
• if reraise is True, catch fatal errors and re-raise them in a pretty way so that borg does not terminate with a traceback, but with a simple, clear error message (which must also contain the original exception text)
• return a boolean whether the directory was created

[Gu1nness]

That's a good idea.
Using makedirs and catching exceptions should be atomic enough to avoid the if if not path exists: makedirs. I just need to list all possible possibilities and catch them accordingly (and find some energy during a burnout…)

[ThomasWaldmann]

@Gu1nness are you working on this? there's a bounty on it now. :-)

[Gu1nness]

Yes, still on this one, I just need to find a way to re-raise the issue in a way that will not leave a stracktrace like you asked (and I got involved in Tor for a few weeks because I wanted to do some C code ^^)
If you have any clues, I'd be happy to hear about it!

EDIT : Found out a few minutes after this message…