Skip to content

Commit

Permalink
tests/valgrind: fix handling of forks
Browse files Browse the repository at this point in the history
We tried to support valgrind checking of fork() in:
    2023-09-20 tests/valgrind: improve handling for fork() and exec()
    38e07b4

However, that method assumed that the first pid would be the parent,
while the second (and subsequent ones) would be the children.  That has
two flaws:

1) if the pid rolls over, the order would be broken.  For example, if
   the parent pid was 99999 on FreeBSD (the maxmimum value), then the
   child would be something much lower (probably in the hundreds).

   (I knew about that problem, considered it sufficiently unlikely.)

2) since $_valgrind_check_logfiles are defined by `ls "blah"*`, they are
   listed in lexicographical order, not numerical order.  For example,
   given two logfiles foo-9999.log and foo-10000.log, foo-10000.log
   would come first.

   (I did not anticipate this problem, but even if it had occurred to
   me, I probably would have deemed it sufficiently unlikely.  However,
   by unlikely fluke, this seems to occur roughly 50% of the time with
   github actions: when 24-fork-func with the gcc binaries, the parent
   lands on pid 9999 exactly and the child is 10000.)

The fix is not to rely on the order: instead, generate a list of all
pids involved.  If a valgrind logfile lists a parent pid which is in
that list, it's a child; otherwise, it's the parent.
  • Loading branch information
gperciva committed Jan 15, 2024
1 parent 9ea50f6 commit 16f2d6d
Showing 1 changed file with 76 additions and 30 deletions.
106 changes: 76 additions & 30 deletions tests/shared_valgrind_functions.sh
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,44 @@ _val_checkl() {
fi
}

## _get_pids (logfiles):
# Extract a list of pids in the format %08d from ${logfiles}.
_get_pids() {
_get_pids_logfiles=$1

_get_pids_pids=""
for _get_pids_logfile in ${_valgrind_check_logfiles} ; do
# Get the pid.
_get_pids_pid=$(printf "%s" "${_get_pids_logfile%%.log}" | \
rev | cut -d "-" -f 1 | rev)
# Zero-pad it and add it to the new list.
_get_pids_pids=$(printf "%s %08d" \
"${_get_pids_pids}" "${_get_pids_pid}")
done

echo "${_get_pids_pids}"
}

## _is_parent (logfile, pids):
# If the parent pid of ${logfile} is in ${pids}, return 0; otherwise, return 1.
_is_parent () {
_is_parent_logfile=$1
_is_parent_pids=$2

# Get the parent pid from the valgrind logfile
ppid=$(grep "Parent PID:" "${_is_parent_logfile}" | \
awk '{ print $4 }')
ppid=$(printf "%08d" "${ppid}")

# If the parent is in the list of pids, this isn't the parent process.
if [ "${_is_parent_pids#*"${ppid}"}" != "${_is_parent_pids}" ] ; then
return 1
fi

# Yes, this is the parent process.
return 0
}

## valgrind_check (exitfile):
# Check for any memory leaks recorded in valgrind logfiles associated with a
# test exitfile. Return the filename if there's a leak; otherwise return an
Expand All @@ -368,41 +406,49 @@ valgrind_check() {
return
fi

# Get a normalized list of pids.
_valgrind_check_pids=$(_get_pids "${_valgrind_check_logfiles}")

# If the valgrind logfiles contain "-valgrind-parent-", then we only
# want to check the parent (the lowest pid).
for _valgrind_check_logfile in ${_valgrind_check_logfiles} ; do
if [ "${_valgrind_check_logfile#*-valgrind-parent-}" != "${_valgrind_check_logfile}" ]; then
# Only check the parent
_val_checkl "${_valgrind_check_logfile}"
return "$?"
fi
done
# want to check the parent. The parent is the logfile whose "parent
# pid" is not in the list of pids. (If one logfile contains
# "-valgrind-parent-" then all of them should have it, so we can
# simply check if that string occurs in the list of logfiles.)
if [ "${_valgrind_check_logfiles#*-valgrind-parent-}" != \
"${_valgrind_check_logfiles}" ]; then
_valgrind_check_parent=1
else
_valgrind_check_parent=0
fi

# If there's two files, there's a fork() -- likely within
# daemonize() -- so only pay attention to the child.
if [ "${_valgrind_check_num}" -eq "2" ]; then
# Find both pids.
_valgrind_check_val_pids=""
for _valgrind_check_logfile in ${_valgrind_check_logfiles} ; do
_valgrind_check_val_pid=$(head -n 1 "${_valgrind_check_logfile}" | cut -d "=" -f 3)
_valgrind_check_val_pids="${_valgrind_check_val_pids} ${_valgrind_check_val_pid}"
done

# Find the logfile which has a parent in the list of pids.
for _valgrind_check_logfile in ${_valgrind_check_logfiles} ; do
_valgrind_check_val_parent_pid=$(grep "Parent PID:" "${_valgrind_check_logfile}" | \
awk '{ print $4 }')
if [ "${_valgrind_check_val_pids#*"${_valgrind_check_val_parent_pid}"}" != \
"${_valgrind_check_val_pids}" ]; then
# Check the logfiles depending on whether it's the parent or not,
# and whether we want to check the parent or children.
for _valgrind_check_logfile in ${_valgrind_check_logfiles} ; do
if _is_parent "${_valgrind_check_logfile}" \
"${_valgrind_check_pids}" ; then
# This is the parent.
if [ "${_valgrind_check_parent}" -eq 1 ] ; then
_val_checkl "${_valgrind_check_logfile}"
return "$?"
# Bail if there's a problem.
rc=$?
if [ "${rc}" -ne 0 ]; then
return "${rc}"
fi
fi
done
fi
else
# This is a child.
if [ "${_valgrind_check_parent}" -eq 0 ] ; then
_val_checkl "${_valgrind_check_logfile}"
# Bail if there's a problem.
rc=$?
if [ "${rc}" -ne 0 ]; then
return "${rc}"
fi
fi
fi
done

# Programmer error; hard bail.
echo "Programmer error: wrong number of valgrind logfiles!" 1>&2
exit 1
return 0
}

## valgrind_init():
Expand Down

0 comments on commit 16f2d6d

Please sign in to comment.