ObjectPackConsumer loops indefinitely on zero-length objects

[mcb30]

The processing loop in ObjectPackConsumer::ConsumePayload():

cvmfs/cvmfs/pack.cc

Lines 338 to 341 in f6095b5

	ObjectPackBuild::State ObjectPackConsumer::ConsumePayload(
	const unsigned buf_size, const unsigned char *buf) {
	uint64_t pos_in_buf = 0;
	while ((pos_in_buf < buf_size) && (idx_ < index_.size())) {

can terminate prematurely when dealing with a zero-length object. Specifically: for a zero-length object at the end of the buffer, pos_in_buf will equal buf_size and so the loop will terminate, but idx_ will not equal index.size() and so the method ends up erroneously returning kStateContinue.

Zero-length objects can occur only when using uncompressed objects, since a compressed object will never have a zero-length compressed representation. The following integration test case triggers the issue:

cvmfs_test_name="Uncompressed files via gateway"
cvmfs_test_autofs_on_startup=false
cvmfs_test_suites="quick"

verify_compression() {
  local filename=$1
  local expected=$2
  local actual=$(attr -qg compression ${filename})
  if [ "${actual}" != "${expected}" ] ; then
    echo "Compression mismatch for ${filename}: expected ${expected} actual ${actual}"
    return 101
  fi
}

cvmfs_run_test() {
  local reponame=test.repo.org
  local repodir=/cvmfs/${reponame}

  echo "set up gateway"
  set_up_repository_gateway || return 1

  echo "parse configuration"
  load_repo_config ${reponame}
  local spooldir=${CVMFS_SPOOL_DIR}

  echo "start transaction"
  cvmfs_server transaction ${reponame} || return 11

  echo "create test files"
  echo "Hello world!" > ${repodir}/file1
  touch ${repodir}/file2

  echo "publish (uncompressed) and check"
  cvmfs_server publish -Z none ${reponame} || return 12
  cvmfs_server check -i ${reponame} || return 13

  echo "verify files"
  [ -f ${repodir}/file1 -a -s ${repodir}/file1 ] || return 14
  [ -f ${repodir}/file2 -a ! -s ${repodir}/file2 ] || return 15
  verify_compression ${spooldir}/rdonly/file1 none || return $?
  verify_compression ${spooldir}/rdonly/file2 none || return $?
}

[mcb30]

A quick hack to ObjectPackConsumer::ConsumePayload() fixes this specific issue:

diff --git a/cvmfs/pack.cc b/cvmfs/pack.cc
index f7d635701..a7f86d56e 100644
--- a/cvmfs/pack.cc
+++ b/cvmfs/pack.cc
@@ -338,7 +338,7 @@ ObjectPackBuild::State ObjectPackConsumer::ConsumeNext(
 ObjectPackBuild::State ObjectPackConsumer::ConsumePayload(
     const unsigned buf_size, const unsigned char *buf) {
   uint64_t pos_in_buf = 0;
-  while ((pos_in_buf < buf_size) && (idx_ < index_.size())) {
+  while ((pos_in_buf <= buf_size) && (idx_ < index_.size())) {
     // Fill the accumulator or process next small object
     uint64_t nbytes;  // How many bytes are consumed in this iteration
     const uint64_t remaining_in_buf = buf_size - pos_in_buf;

but I have not yet reviewed this change to see if it might create other failure cases.

[mcb30]

but I have not yet reviewed this change to see if it might create other failure cases.

That hack would (I think) have caused an infinite loop in the case of a partial object. I've created what should be a proper fix in #2731