tags 723957 + patch thanks Hi Matt,
Thanks for reporting and triaging this bug. I'm sorry no one has answered it until now. On 21/09/13 09:13 AM, Matt Brown wrote:
The get_directory method used in several maint scripts contains a bug that causes it to return multiple lines of output if a commented olcDbDirectory line also exists in the configuration file.
First, I should mention that manually editing the files belonging to cn=config is explicitly not supported by upstream, and newer openldap versions have notices to that effect in those files as well as logging warnings if manual edits are detected (by a CRC check). Of course, that doesn't make this upgrade failure any less of a bug.
The callers of get_directory use filesystem existence checks on the output of get_directory to determine whether to actually backup the database, and silently continue without backing up when multiple lines of output are returned.
The following patch (anchoring the match to start of line) would be a minimal fix for this critical issue, but a more proper fix would be for the preinst to bail out if it is unable to actually backup a database that it knows to exist from the config!
Agreed. I want to avoid re-introducing #584712, but I think checking whether the olcDbDirectory attribute is present (the [ -n ] test) is sufficient to decide whether a database has local files that need to be backed up. The [ -d ] test lets cases like this go by silently, and there's already code shortly after for catching the slapcat failure if something actually does go wrong. So I'd propose this change in addition to your suggestion: --- slapd.preinst +++ slapd.preinst @@ -173,7 +173,7 @@ echo >&2 " Dumping to $dir: " (get_suffix | while read suffix; do dbdir=`get_directory "$suffix"` - if [ -n "$dbdir" ] && [ -d "$dbdir" ]; then + if [ -n "$dbdir" ]; then file="$dir/$suffix.ldif" echo -n " - directory $suffix... " >&2 # Need to support slapd.d migration from preinst The anchoring bug you noticed is present in a few other spots too, affecting for example olcSuffix. Attached is a patch combining all the mentioned changes. It lets me complete an upgrade when cn=config's files contain commented olcSuffix and olcDbDirectory lines as well as a second database with no olcDbDirectory, and falls through to the slapcat-failure case if the DB directory is inaccessible. IMO in general the maintainer scripts should be using slapcat to query the config DB and not just looking into the files directly, which could avoid this sort of issue, but there are probably implications I haven't thought of.
diff -Nru openldap-2.4.39/debian/slapd.scripts-common openldap-2.4.39/debian/slapd.scripts-common --- openldap-2.4.39/debian/slapd.scripts-common 2014-04-05 16:35:04.000000000 -0700 +++ openldap-2.4.39/debian/slapd.scripts-common 2014-04-05 16:33:38.000000000 -0700 @@ -165,7 +165,7 @@ echo >&2 " Dumping to $dir: " (get_suffix | while read suffix; do dbdir=`get_directory "$suffix"` - if [ -n "$dbdir" ] && [ -d "$dbdir" ]; then + if [ -n "$dbdir" ]; then file="$dir/$suffix.ldif" echo -n " - directory $suffix... " >&2 # Need to support slapd.d migration from preinst @@ -275,14 +275,14 @@ sed -n -e's/^suffix[[:space:]]\+"*\([^"]\+\)"*/\1/p' $f done else - grep -h olcSuffix ${SLAPD_CONF}/cn\=config/olcDatabase*.ldif | cut -d: -f 2 + grep -h ^olcSuffix ${SLAPD_CONF}/cn\=config/olcDatabase*.ldif | cut -d: -f 2 fi } # }}} get_directory() { # {{{ # Returns the db directory for a given suffix if [ -d "${SLAPD_CONF}" ] && get_suffix | grep -q "$1" ; then - grep "olcDbDirectory:" `grep -l "olcSuffix: $1" ${SLAPD_CONF}/cn\=config/olcDatabase*.ldif` | cut -d: -f 2 | sed 's/^ *//g' + grep "^olcDbDirectory:" `grep -l "^olcSuffix: $1" ${SLAPD_CONF}/cn\=config/olcDatabase*.ldif` | cut -d: -f 2 | sed 's/^ *//g' elif [ -f "${SLAPD_CONF}" ]; then # Extract the directory for the given suffix ($1) for f in `get_all_slapd_conf_files`; do