Item14200: Potentially thread unsafe code in LdapContrib::refreshCache

I'm looking at:

sub refreshCache {
  my ($this, $mode) = @_;

  return unless $mode;

  $this->{_refreshMode} = $mode;

# create a temporary tie
  my $tempCacheFile = $this->{cacheFile} . '_tmp';
  if (-e $tempCacheFile) {
    writeWarning("already refreshing cache");
    return 0;
  }

  my %tempData;
  my $tempCache = tie %tempData, 'DB_File', $tempCacheFile, O_CREAT | O_RDWR, 0664, $DB_HASH
    or die "Cannot open file $tempCacheFile: $!";

My understanding may be flawed. I believe tie is atomic, but DB_File is not DB_File::Lock, and O_CREAT will open an existing file unless you have O_EXCL. If two requests make it past the -e $tempCacheFile together, one will create the file and the other will open it but both will succeed and have access. Disaster may follow.

This also worries me:

$this->untieCache();

  #writeDebug("replacing working copy");
  rename $tempCacheFile, $this->{cacheFile};

  # reconnect hash
  $this->tieCache('read');

Is rename atomic? Does it respect locks other processes might have on the file?

I would like these to be the cause of a formerly-once-a-year bug that corrupts groups on my system which has shown up twice in the past three weeks, it would mean I could stop looking.

-- FredTarbox - 17 Oct 2016

Setting to Confirmed. Jast has confirmed that there are indeed some thread safe issues in LdapContrib.

-- GeorgeClark - 02 Dec 2016

The real fix would be to not use BerkeleyDB for concurrent updates ... which isn't happening regularly anyway.

For now, the best way to avoid a race condition is to not update the cache online using an appropriate negative MaxCacheAge value and update the cache using a cron job at midnight. This is actually so highly recommended that I think of disabling online updates all together, means: you must use a cron job to update the ldap cache.

-- MichaelDaum - 30 Aug 2017

Commenting is disabled while not logged in