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
