From 0beb885cbf699ada428254b61ea2485b8e4d9c7e Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Tue, 9 Sep 2025 17:31:57 +1000 Subject: [PATCH 1/2] compare: properly compare tar_time and time In certain circumstances, if a manifest with "time" keywords was compared to another manifest with "tar_time" keywords, differences were not actually reported despite there being logic for it. Unfortunately, even though commit 26ff922da64a ("compare: implement mtree.DirectoryHierarchy comparisons") explicitly included logic to handle the necessary "time" -> "tar_time" conversions, the same commit also made it so that Compare() could be instructed to only consider a subset of keywords and failed to take into account said "time" -> "tar_time" remapping. Most users have likely not noticed this because gomtree will re-use the keywords from a manifest when doing a straightforward "gomtree validate" invocation, but if you explicitly requested "time" when validating a "tar_time" manifest (and *not* the other way around) then any time changes would be undetected. Fixes: 26ff922da64a ("compare: implement mtree.DirectoryHierarchy comparisons") Signed-off-by: Aleksa Sarai --- compare.go | 18 ++++++++------ test/cli/0012.sh | 64 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 8 deletions(-) create mode 100644 test/cli/0012.sh diff --git a/compare.go b/compare.go index 29da1ab..b73f174 100644 --- a/compare.go +++ b/compare.go @@ -3,6 +3,7 @@ package mtree import ( "encoding/json" "fmt" + "slices" "strconv" ) @@ -405,15 +406,16 @@ func compare(oldDh, newDh *DirectoryHierarchy, keys []Keyword, same bool) ([]Ino return nil, fmt.Errorf("comparison failed %s: %s", path, err) } - // Now remove "changed" entries that don't match the keys. + // Ignore changes to keys not in the requested set. if keys != nil { - var filterChanged []KeyDelta - for _, keyDiff := range changed { - if InKeywordSlice(keyDiff.name.Prefix(), keys) { - filterChanged = append(filterChanged, keyDiff) - } - } - changed = filterChanged + changed = slices.DeleteFunc(changed, func(delta KeyDelta) bool { + name := delta.name.Prefix() + return !InKeywordSlice(name, keys) && + // We remap time to tar_time in compareEntry, so we + // need to treat them equivalently here. + !(name == "time" && InKeywordSlice("tar_time", keys)) && + !(name == "tar_time" && InKeywordSlice("time", keys)) + }) } // Check if there were any actual changes. diff --git a/test/cli/0012.sh b/test/cli/0012.sh new file mode 100644 index 0000000..cd4960b --- /dev/null +++ b/test/cli/0012.sh @@ -0,0 +1,64 @@ +#!/bin/bash +set -ex + +name=$(basename $0) +root="$(dirname $(dirname $(dirname $0)))" +gomtree=$(go run ${root}/test/realpath/main.go ${root}/gomtree) +t=$(mktemp -d /tmp/go-mtree.XXXXXX) + +echo "[${name}] Running in ${t}" + +## Make sure that comparing manifests with tar_time and time correctly detect +## errors (while also *truncating* timestamps for tar_time comparisons). + +pushd ${root} +mkdir -p ${t}/root + +date="2025-09-05T13:05:10.12345" # POSIX format for "touch -d". + +touch -d "$date" ${t}/root/foo + +keywords=type,uid,gid,nlink,link,mode,flags,xattr,size,sha256 + +# Generate regular manifests with time and tar_time. +${gomtree} -c -k ${keywords},time -p ${t}/root -f ${t}/time.mtree +${gomtree} -c -k ${keywords},tar_time -p ${t}/root -f ${t}/tartime.mtree + +# Validation with both manifests should still succeed. +${gomtree} validate -p ${t}/root -f ${t}/time.mtree +${gomtree} validate -p ${t}/root -f ${t}/tartime.mtree +# Manifest comparison should also succeed. +${gomtree} validate -f ${t}/tartime.mtree -f ${t}/time.mtree +${gomtree} validate -f ${t}/time.mtree -f ${t}/tartime.mtree +# Forcefully requesting a different time type should also succeed. +${gomtree} validate -k ${keywords},tar_time -p ${t}/root -f ${t}/time.mtree +${gomtree} validate -k ${keywords},time -p ${t}/root -f ${t}/tartime.mtree + +# Change the timestamp. +touch -d "1997-03-25T13:40:00.67890" ${t}/root/foo + +# Standard validations should fail. +(! ${gomtree} validate -p ${t}/root -f ${t}/time.mtree) +(! ${gomtree} validate -p ${t}/root -f ${t}/tartime.mtree) + +# Forcefully requesting a different time type should also fail. +(! ${gomtree} validate -k ${keywords},tar_time -p ${t}/root -f ${t}/time.mtree) +(! ${gomtree} validate -k ${keywords},time -p ${t}/root -f ${t}/tartime.mtree) + +# Ditto if we generate the manifests and compare them. +${gomtree} -c -k ${keywords},time -p ${t}/root -f ${t}/time-change.mtree +${gomtree} -c -k ${keywords},tar_time -p ${t}/root -f ${t}/tartime-change.mtree + +# Same time types. +(! ${gomtree} validate -f ${t}/time.mtree -f ${t}/time-change.mtree) +(! ${gomtree} validate -f ${t}/time-change.mtree -f ${t}/time.mtree) +(! ${gomtree} validate -f ${t}/tartime.mtree -f ${t}/tartime-change.mtree) +(! ${gomtree} validate -f ${t}/tartime-change.mtree -f ${t}/tartime.mtree) + +# Different time types: +# (old) time <=> (new) tar_time +(! ${gomtree} validate -f ${t}/time.mtree -f ${t}/tartime-change.mtree) +(! ${gomtree} validate -f ${t}/tartime-change.mtree -f ${t}/time.mtree) +# (old) tar_time <=> (new) time +(! ${gomtree} validate -f ${t}/tartime.mtree -f ${t}/time-change.mtree) +(! ${gomtree} validate -f ${t}/time-change.mtree -f ${t}/tartime.mtree) From 604ab428638cf6515f0734306ca1c6142de1c047 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Tue, 9 Sep 2025 17:55:02 +1000 Subject: [PATCH 2/2] compare: improve tar_time truncation Rather than parsing the value as a float and then truncating it, just parse it as an integer in the first place (this also adds some validation that we are parsing a reasonable-looking value). While we're at it, add some integration tests for this code to make sure this quite complicated special-case behaviour doesn't regress. Signed-off-by: Aleksa Sarai --- compare.go | 29 +++++++++---- test/cli/0012trunc.sh | 96 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 116 insertions(+), 9 deletions(-) create mode 100644 test/cli/0012trunc.sh diff --git a/compare.go b/compare.go index b73f174..81aba20 100644 --- a/compare.go +++ b/compare.go @@ -4,7 +4,6 @@ import ( "encoding/json" "fmt" "slices" - "strconv" ) // XXX: Do we need a Difference interface to make it so people can do var x @@ -253,23 +252,35 @@ func compareEntry(oldEntry, newEntry Entry) ([]KeyDelta, error) { // Make a new tar_time. if diffs["tar_time"].Old == nil { - time, err := strconv.ParseFloat(timeStateT.Old.Value(), 64) - if err != nil { - return nil, fmt.Errorf("failed to parse old time: %s", err) + var ( + timeSec, timeNsec int64 + // used to check for trailing characters + trailing rune + ) + val := timeStateT.Old.Value() + n, _ := fmt.Sscanf(val, "%d.%d%c", &timeSec, &timeNsec, &trailing) + if n != 2 { + return nil, fmt.Errorf("failed to parse old time: invalid format %q", val) } newTime := new(KeyVal) - *newTime = KeyVal(fmt.Sprintf("tar_time=%d.000000000", int64(time))) + *newTime = KeyVal(fmt.Sprintf("tar_time=%d.%9.9d", timeSec, 0)) diffs["tar_time"].Old = newTime } else if diffs["tar_time"].New == nil { - time, err := strconv.ParseFloat(timeStateT.New.Value(), 64) - if err != nil { - return nil, fmt.Errorf("failed to parse new time: %s", err) + var ( + timeSec, timeNsec int64 + // used to check for trailing characters + trailing rune + ) + val := timeStateT.New.Value() + n, _ := fmt.Sscanf(val, "%d.%d%c", &timeSec, &timeNsec, &trailing) + if n != 2 { + return nil, fmt.Errorf("failed to parse new time: invalid format %q", val) } newTime := new(KeyVal) - *newTime = KeyVal(fmt.Sprintf("tar_time=%d.000000000", int64(time))) + *newTime = KeyVal(fmt.Sprintf("tar_time=%d.%9.9d", timeSec, 0)) diffs["tar_time"].New = newTime } else { diff --git a/test/cli/0012trunc.sh b/test/cli/0012trunc.sh new file mode 100644 index 0000000..a9a7459 --- /dev/null +++ b/test/cli/0012trunc.sh @@ -0,0 +1,96 @@ +#!/bin/bash +set -ex + +name=$(basename $0) +root="$(dirname $(dirname $(dirname $0)))" +gomtree=$(go run ${root}/test/realpath/main.go ${root}/gomtree) +t=$(mktemp -d /tmp/go-mtree.XXXXXX) + +echo "[${name}] Running in ${t}" + +## Make sure that comparing manifests with tar_time and time are correctly +## truncated. + +pushd ${root} +mkdir -p ${t}/root + +date="2025-09-05T13:05:10" # POSIX format for "touch -d". + +echo "less than .5" >${t}/root/lowerhalf +touch -d "$date.1000" ${t}/root/lowerhalf +echo "more than .5" >${t}/root/upperhalf +touch -d "$date.8000" ${t}/root/upperhalf +echo "no subsecond" >${t}/root/tartime +touch -d "$date.0000" ${t}/root/tartime + +keywords=type,uid,gid,nlink,link,mode,flags,xattr,size,sha256 + +# Generate regular manifests with time and tar_time. +${gomtree} -c -k ${keywords},time -p ${t}/root -f ${t}/time.mtree +${gomtree} -c -k ${keywords},tar_time -p ${t}/root -f ${t}/tartime.mtree + +# Make sure that tar_time truncates the value. +unix="$(date -d ${date} +%s)" +grep -q "lowerhalf.*tar_time=$unix.000000000" ${t}/tartime.mtree +grep -q "upperhalf.*tar_time=$unix.000000000" ${t}/tartime.mtree +grep -q "tartime.*tar_time=$unix.000000000" ${t}/tartime.mtree + +# Validation with both manifests should still succeed. +${gomtree} validate -p ${t}/root -f ${t}/time.mtree +${gomtree} validate -p ${t}/root -f ${t}/tartime.mtree +# Manifest comparison should also succeed. +${gomtree} validate -f ${t}/tartime.mtree -f ${t}/time.mtree +${gomtree} validate -f ${t}/time.mtree -f ${t}/tartime.mtree + +# Truncate the on-disk timestamps manually. +touch -d "$date.0000" ${t}/root/lowerhalf +touch -d "$date.0000" ${t}/root/upperhalf +touch -d "$date.0000" ${t}/root/tartime + +# Only the tar_time manifest should succeed. +(! ${gomtree} validate -p ${t}/root -f ${t}/time.mtree) +${gomtree} validate -p ${t}/root -f ${t}/tartime.mtree +${gomtree} validate -k ${keywords},time -p ${t}/root -f ${t}/tartime.mtree +# ... unless you force the usage of tar_time. +${gomtree} validate -k ${keywords},tar_time -p ${t}/root -f ${t}/time.mtree + +# The same goes for if you generate the manifests and compare them instead. +${gomtree} -c -k ${keywords},time -p ${t}/root -f ${t}/time-trunc.mtree +${gomtree} -c -k ${keywords},tar_time -p ${t}/root -f ${t}/tartime-trunc.mtree +# Comparing time with time should fail ... +(! ${gomtree} validate -f ${t}/time.mtree -f ${t}/time-trunc.mtree) +(! ${gomtree} validate -f ${t}/time-trunc.mtree -f ${t}/time.mtree) +# ... tar_time with tar_time should succeed ... +${gomtree} validate -f ${t}/tartime.mtree -f ${t}/tartime-trunc.mtree +${gomtree} validate -f ${t}/tartime-trunc.mtree -f ${t}/tartime.mtree +# ... old tar_time with new time should succeed ... +${gomtree} validate -f ${t}/tartime.mtree -f ${t}/time-trunc.mtree +${gomtree} validate -f ${t}/time-trunc.mtree -f ${t}/tartime.mtree +# ... and new tar_time against old time should succeed. +${gomtree} validate -f ${t}/tartime-trunc.mtree -f ${t}/time.mtree +${gomtree} validate -f ${t}/time.mtree -f ${t}/tartime-trunc.mtree + +# Change the timestamp entirely. +touch -d "1997-03-25T13:40:00" ${t}/root/lowerhalf +touch -d "1997-03-25T13:40:00" ${t}/root/upperhalf +touch -d "1997-03-25T13:40:00" ${t}/root/tartime + +# Now all validations should fail. +(! ${gomtree} validate -p ${t}/root -f ${t}/time.mtree) +(! ${gomtree} validate -p ${t}/root -f ${t}/tartime.mtree) +(! ${gomtree} validate -k ${keywords},tar_time -p ${t}/root -f ${t}/time.mtree) +(! ${gomtree} validate -k ${keywords},time -p ${t}/root -f ${t}/tartime.mtree) + +# Ditto for generating the manifests and comparing them. +${gomtree} -c -k ${keywords},time -p ${t}/root -f ${t}/time-change.mtree +${gomtree} -c -k ${keywords},tar_time -p ${t}/root -f ${t}/tartime-change.mtree + +# Try all combinations. +lefts=( ${t}/{tar,}time{,-trunc}.mtree ) +rights=( ${t}/{tar,}time-change.mtree ) +for left in "${lefts[@]}"; do + for right in "${rights[@]}"; do + (! ${gomtree} validate -f ${left} -f ${right}) + (! ${gomtree} validate -f ${right} -f ${left}) + done +done