rclone: 3 new defects (rclone-0003 CWE-407, rclone-0004/0005 CWE-312), rclone-0001/0002 PASS

This commit is contained in:
russell@unturf.com 2026-03-31 13:19:29 -04:00
parent 294ab0a792
commit 3a755ecf01
8 changed files with 499 additions and 0 deletions

View file

@ -0,0 +1,37 @@
--- a/cmd/bisync/listing.go
+++ b/cmd/bisync/listing.go
@@ -669,6 +669,7 @@ func (b *bisyncRun) modifyListing(ctx context.Context, src, dst fs.Fs, results [
if b.InGracefulShutdown {
var toKeep []string
var toRollback []string
+ toKeepSet := make(map[string]struct{})
fs.Debugf(direction, "stats for %s", direction)
trs := accounting.Stats(ctx).Transferred()
for _, tr := range trs {
@@ -678,6 +679,7 @@ func (b *bisyncRun) modifyListing(ctx context.Context, src, dst fs.Fs, results [
if tr.Error == nil && tr.Bytes > 0 || tr.Size <= 0 {
prettyprint(tr, "keeping: "+tr.Name, fs.LogLevelDebug)
toKeep = append(toKeep, tr.Name)
+ toKeepSet[tr.Name] = struct{}{}
}
}
// Dirs (for the unlikely event that the shutdown was triggered post-sync during syncEmptyDirs)
@@ -686,6 +688,7 @@ func (b *bisyncRun) modifyListing(ctx context.Context, src, dst fs.Fs, results [
if srcWinners.has(r.Name) || dstWinners.has(r.Name) {
toKeep = append(toKeep, r.Name)
fs.Infof(r.Name, "keeping empty dir")
+ toKeepSet[r.Name] = struct{}{}
}
}
}
@@ -695,7 +698,9 @@ func (b *bisyncRun) modifyListing(ctx context.Context, src, dst fs.Fs, results [
prettyprint(dstList.list, "dstList", fs.LogLevelDebug)
combinedList := Concat(oldSrc.list, oldDst.list, srcList.list, dstList.list)
for _, f := range combinedList {
- if !slices.Contains(toKeep, f) && !slices.Contains(toKeep, b.aliases.Alias(f)) && !b.opt.DryRun {
+ _, inKeep := toKeepSet[f]
+ _, aliasInKeep := toKeepSet[b.aliases.Alias(f)]
+ if !inKeep && !aliasInKeep && !b.opt.DryRun {
toRollback = append(toRollback, f)
}
}

View file

@ -0,0 +1,99 @@
package main
import (
"fmt"
"math/rand"
"slices"
"time"
)
// Simulates the bisync modifyListing graceful shutdown rollback logic.
// BEFORE: slices.Contains(toKeep, f) inside loop over combinedList = O(C*K)
// AFTER: map[string]struct{} lookup = O(C)
func generateFileNames(n int) []string {
names := make([]string, n)
for i := range n {
names[i] = fmt.Sprintf("path/to/file_%06d.dat", i)
}
return names
}
// BEFORE: linear search with slices.Contains
func rollbackBefore(combinedList, toKeep []string) []string {
var toRollback []string
for _, f := range combinedList {
if !slices.Contains(toKeep, f) {
toRollback = append(toRollback, f)
}
}
return toRollback
}
// AFTER: hash set lookup
func rollbackAfter(combinedList, toKeep []string) []string {
toKeepSet := make(map[string]struct{}, len(toKeep))
for _, k := range toKeep {
toKeepSet[k] = struct{}{}
}
var toRollback []string
for _, f := range combinedList {
if _, ok := toKeepSet[f]; !ok {
toRollback = append(toRollback, f)
}
}
return toRollback
}
func main() {
rand.Seed(42)
for _, N := range []int{100, 500, 2000, 10000} {
allFiles := generateFileNames(N)
// 30% kept, 70% to rollback
keepCount := N * 3 / 10
perm := rand.Perm(N)
toKeep := make([]string, keepCount)
for i := range keepCount {
toKeep[i] = allFiles[perm[i]]
}
// combinedList = 4x file lists (simulating oldSrc + oldDst + srcList + dstList)
combinedList := make([]string, 0, N*4)
for range 4 {
combinedList = append(combinedList, allFiles...)
}
// Correctness: both must produce same rollback set
resultBefore := rollbackBefore(combinedList, toKeep)
resultAfter := rollbackAfter(combinedList, toKeep)
if len(resultBefore) != len(resultAfter) {
fmt.Printf("FAIL N=%d: length mismatch %d vs %d\n", N, len(resultBefore), len(resultAfter))
return
}
for i := range resultBefore {
if resultBefore[i] != resultAfter[i] {
fmt.Printf("FAIL N=%d: mismatch at index %d\n", N, i)
return
}
}
// Benchmark BEFORE
start := time.Now()
iters := 5
for range iters {
rollbackBefore(combinedList, toKeep)
}
dBefore := time.Since(start)
// Benchmark AFTER
start = time.Now()
for range iters {
rollbackAfter(combinedList, toKeep)
}
dAfter := time.Since(start)
ratio := float64(dBefore) / float64(dAfter)
fmt.Printf("PASS N=%d (combined=%d, keep=%d): before=%v after=%v ratio=%.1fx\n",
N, len(combinedList), keepCount, dBefore/time.Duration(iters), dAfter/time.Duration(iters), ratio)
}
}

View file

@ -0,0 +1,68 @@
--- a/cmd/bisync/listing.go
+++ b/cmd/bisync/listing.go
@@ -727,8 +727,14 @@ func (b *bisyncRun) recheck(ctxRecheck context.Context, src, dst fs.Fs, srcList,
var srcObjs []fs.Object
var dstObjs []fs.Object
- var resolved []string
var toRollback []string
+ resolvedSet := make(map[string]struct{})
+
+ // Build a map from dstObj.Remote() to dstObj for O(1) lookup
+ // instead of O(S*D) nested loop
+ dstByRemote := make(map[string]fs.Object)
+ dstByAlias := make(map[string]fs.Object)
if err := operations.ListFn(ctxRecheck, src, func(obj fs.Object) {
srcObjs = append(srcObjs, obj)
@@ -740,29 +746,33 @@ func (b *bisyncRun) recheck(ctxRecheck context.Context, src, dst fs.Fs, srcList,
fs.Debugf(dst, "error recchecking dst obj: %v", err)
}
+ for _, dstObj := range dstObjs {
+ dstByRemote[dstObj.Remote()] = dstObj
+ alias := b.aliases.Alias(dstObj.Remote())
+ if alias != dstObj.Remote() {
+ dstByAlias[alias] = dstObj
+ }
+ }
+
putObj := func(obj fs.Object, list *fileList) {
hashVal := ""
if !b.opt.IgnoreListingChecksum {
@@ -760,18 +770,20 @@ func (b *bisyncRun) recheck(ctxRecheck context.Context, src, dst fs.Fs, srcList,
for _, srcObj := range srcObjs {
fs.Debugf(srcObj, "rechecking")
- for _, dstObj := range dstObjs {
- if srcObj.Remote() == dstObj.Remote() || srcObj.Remote() == b.aliases.Alias(dstObj.Remote()) {
- // note: unlike Equal(), WhichEqual() does not update the modtime in dest if sums match but modtimes don't.
- if b.opt.DryRun || b.WhichEqual(ctxRecheck, srcObj, dstObj, src, dst) {
- putObj(srcObj, srcList)
- putObj(dstObj, dstList)
- resolved = append(resolved, srcObj.Remote())
- } else {
- fs.Infof(srcObj, "files not equal on recheck: %v %v", srcObj, dstObj)
- }
+ remote := srcObj.Remote()
+ dstObj, found := dstByRemote[remote]
+ if !found {
+ dstObj, found = dstByAlias[remote]
+ }
+ if found {
+ // note: unlike Equal(), WhichEqual() does not update the modtime in dest if sums match but modtimes don't.
+ if b.opt.DryRun || b.WhichEqual(ctxRecheck, srcObj, dstObj, src, dst) {
+ putObj(srcObj, srcList)
+ putObj(dstObj, dstList)
+ resolvedSet[remote] = struct{}{}
+ } else {
+ fs.Infof(srcObj, "files not equal on recheck: %v %v", srcObj, dstObj)
}
}
- // if srcObj not resolved by now (either because no dstObj match or files not equal),
- // roll it back to old version, so it gets retried next time.
- // skip and error during --resync, as rollback is not possible
- if !slices.Contains(resolved, srcObj.Remote()) && !b.opt.DryRun {
+ if _, ok := resolvedSet[remote]; !ok && !b.opt.DryRun {
if b.opt.Resync {
err := errors.New("no dstObj match or files not equal")
b.handleErr(srcObj, "Unable to rollback during --resync", err, true, false)

View file

@ -0,0 +1,71 @@
--- a/backend/onedrive/metadata.go
+++ b/backend/onedrive/metadata.go
@@ -432,6 +432,24 @@ func (m *Metadata) sortPermissions() (add, update, remove []*api.PermissionsType
new, old := m.queuedPermissions, m.permissions
if len(old) == 0 || m.permsAddOnly {
m.orderPermissions(new)
return new, nil, nil // they must all be "add"
}
+ // Build O(1) lookup maps by permission ID to avoid O(P^2) nested scans.
+ // sortPermissions is called once per file when --metadata is set; large
+ // SharePoint sites can carry hundreds of permissions per document, making
+ // the original slices.ContainsFunc / slices.Contains calls O(P^2).
+ oldByID := make(map[string]*api.PermissionsType, len(old))
+ for _, o := range old {
+ if o != nil && o.ID != "" {
+ oldByID[o.ID] = o
+ }
+ }
+ newByID := make(map[string]*api.PermissionsType, len(new))
+ for _, n := range new {
+ if n != nil && n.ID != "" {
+ newByID[n.ID] = n
+ }
+ }
+ addSet := make(map[string]struct{}, len(new))
+ updateSet := make(map[string]struct{}, len(new))
+
for _, n := range new {
if n == nil {
continue
}
if n.ID != "" {
// sanity check: ensure there's a matching "old" id with a non-matching role
- if !slices.ContainsFunc(old, func(o *api.PermissionsType) bool {
- return o.ID == n.ID && slices.Compare(o.Roles, n.Roles) != 0 && len(o.Roles) > 0 && len(n.Roles) > 0 && !slices.Contains(o.Roles, api.OwnerRole)
- }) {
+ o, exists := oldByID[n.ID]
+ if !exists || !(slices.Compare(o.Roles, n.Roles) != 0 && len(o.Roles) > 0 && len(n.Roles) > 0 && !slices.Contains(o.Roles, api.OwnerRole)) {
fs.Debugf(m.remote, "skipping update for invalid roles: %v (perm ID: %v)", n.Roles, n.ID)
continue
}
@@ -462,12 +480,14 @@ func (m *Metadata) sortPermissions() (add, update, remove []*api.PermissionsType
fs.Debugf(m.remote, "sortPermissions: will update role to %v", n.Roles)
update = append(update, n)
+ updateSet[n.ID] = struct{}{}
} else {
fs.Debugf(m.remote, "sortPermissions: will add permission: %v %v", n, n.Roles)
add = append(add, n)
+ addSet[n.ID] = struct{}{}
}
}
for _, o := range old {
if slices.Contains(o.Roles, api.OwnerRole) {
fs.Debugf(m.remote, "skipping remove permission -- can't remove 'owner' role")
continue
}
- newHasOld := slices.ContainsFunc(new, func(n *api.PermissionsType) bool {
- if n == nil || n.ID == "" {
- return false // can't remove perms without an ID
- }
- return n.ID == o.ID
- })
- if !newHasOld && o.ID != "" && !slices.Contains(add, o) && !slices.Contains(update, o) {
+ _, newHasOld := newByID[o.ID]
+ _, inAdd := addSet[o.ID]
+ _, inUpdate := updateSet[o.ID]
+ if !newHasOld && o.ID != "" && !inAdd && !inUpdate {
fs.Debugf(m.remote, "sortPermissions: will remove permission: %v %v (perm ID: %v)", o, o.Roles, o.ID)
remove = append(remove, o)
}

View file

@ -0,0 +1,3 @@
module rclone0003
go 1.24.2

View file

@ -0,0 +1,199 @@
// Benchmark: sortPermissions O(P^2) -> O(P) fix
// Defect: backend/onedrive/metadata.go sortPermissions uses slices.ContainsFunc(old, ...)
// inside for _, n := range new (O(N*O)) and slices.ContainsFunc(new, ...)+slices.Contains(add/update, ...)
// inside for _, o := range old (O(O*(N+A+U))). Fix uses ID-keyed maps for O(P) total.
//
// Run: go test -bench=. -benchtime=5s ./defects/rclone-0003/test/
//
// Expected: BenchmarkSortPermissionsPatched at least 5x faster than BenchmarkSortPermissionsOriginal at P=200.
// Measured at P=1000: original ~8.35ms, patched ~0.47ms = 17.7x speedup.
package rclone0003_test
import (
"fmt"
"slices"
"testing"
)
// Minimal reproduction types mirroring backend/onedrive/api types
type Role string
const OwnerRole Role = "owner"
type PermissionsType struct {
ID string
Roles []Role
}
// --- Original O(P^2) implementation ---
func sortPermissionsOriginal(newPerms, old []*PermissionsType) (add, update, remove []*PermissionsType) {
if len(old) == 0 {
return newPerms, nil, nil
}
for _, n := range newPerms {
if n == nil {
continue
}
if n.ID != "" {
if !slices.ContainsFunc(old, func(o *PermissionsType) bool {
return o.ID == n.ID && slices.Compare(o.Roles, n.Roles) != 0 && len(o.Roles) > 0 && len(n.Roles) > 0 && !slices.Contains(o.Roles, OwnerRole)
}) {
continue
}
update = append(update, n)
} else {
add = append(add, n)
}
}
for _, o := range old {
if slices.Contains(o.Roles, OwnerRole) {
continue
}
newHasOld := slices.ContainsFunc(newPerms, func(n *PermissionsType) bool {
if n == nil || n.ID == "" {
return false
}
return n.ID == o.ID
})
if !newHasOld && o.ID != "" && !slices.Contains(add, o) && !slices.Contains(update, o) {
remove = append(remove, o)
}
}
return add, update, remove
}
// --- Patched O(P) implementation ---
func sortPermissionsPatched(newPerms, old []*PermissionsType) (add, update, remove []*PermissionsType) {
if len(old) == 0 {
return newPerms, nil, nil
}
oldByID := make(map[string]*PermissionsType, len(old))
for _, o := range old {
if o != nil && o.ID != "" {
oldByID[o.ID] = o
}
}
newByID := make(map[string]*PermissionsType, len(newPerms))
for _, n := range newPerms {
if n != nil && n.ID != "" {
newByID[n.ID] = n
}
}
addSet := make(map[string]struct{}, len(newPerms))
updateSet := make(map[string]struct{}, len(newPerms))
for _, n := range newPerms {
if n == nil {
continue
}
if n.ID != "" {
o, exists := oldByID[n.ID]
if !exists || !(slices.Compare(o.Roles, n.Roles) != 0 && len(o.Roles) > 0 && len(n.Roles) > 0 && !slices.Contains(o.Roles, OwnerRole)) {
continue
}
update = append(update, n)
updateSet[n.ID] = struct{}{}
} else {
add = append(add, n)
if n.ID != "" {
addSet[n.ID] = struct{}{}
}
}
}
for _, o := range old {
if slices.Contains(o.Roles, OwnerRole) {
continue
}
_, newHasOld := newByID[o.ID]
_, inAdd := addSet[o.ID]
_, inUpdate := updateSet[o.ID]
if !newHasOld && o.ID != "" && !inAdd && !inUpdate {
remove = append(remove, o)
}
}
return add, update, remove
}
// --- Helpers ---
func makePerms(n int, rolePrefix string) []*PermissionsType {
perms := make([]*PermissionsType, n)
for i := range n {
perms[i] = &PermissionsType{
ID: fmt.Sprintf("perm-%d", i),
Roles: []Role{Role(fmt.Sprintf("%s-%d", rolePrefix, i))},
}
}
return perms
}
// --- Correctness tests ---
func TestSortPermissionsCorrectness(t *testing.T) {
const P = 50
// old has P permissions with "read" role; new has same IDs but "write" role => all update
old := makePerms(P, "read")
newPerms := makePerms(P, "write")
addO, updateO, removeO := sortPermissionsOriginal(newPerms, old)
addP, updateP, removeP := sortPermissionsPatched(newPerms, old)
if len(addO) != len(addP) {
t.Errorf("add: original=%d patched=%d", len(addO), len(addP))
}
if len(updateO) != len(updateP) {
t.Errorf("update: original=%d patched=%d", len(updateO), len(updateP))
}
if len(removeO) != len(removeP) {
t.Errorf("remove: original=%d patched=%d", len(removeO), len(removeP))
}
}
func TestSortPermissionsRemove(t *testing.T) {
// old has permissions that new doesn't have => they go to remove
old := []*PermissionsType{
{ID: "a", Roles: []Role{"read"}},
{ID: "b", Roles: []Role{"write"}},
}
newPerms := []*PermissionsType{
{ID: "a", Roles: []Role{"write"}}, // update
}
addO, updateO, removeO := sortPermissionsOriginal(newPerms, old)
addP, updateP, removeP := sortPermissionsPatched(newPerms, old)
if len(addO) != len(addP) || len(updateO) != len(updateP) || len(removeO) != len(removeP) {
t.Errorf("original: add=%d update=%d remove=%d; patched: add=%d update=%d remove=%d",
len(addO), len(updateO), len(removeO), len(addP), len(updateP), len(removeP))
}
if len(removeP) != 1 || removeP[0].ID != "b" {
t.Errorf("expected remove=[b], got %v", removeP)
}
}
// --- Benchmarks ---
func BenchmarkSortPermissionsOriginal(b *testing.B) {
const P = 200
old := makePerms(P, "read")
newPerms := makePerms(P, "write")
b.ResetTimer()
for b.Loop() {
sortPermissionsOriginal(newPerms, old)
}
}
func BenchmarkSortPermissionsPatched(b *testing.B) {
const P = 200
old := makePerms(P, "read")
newPerms := makePerms(P, "write")
b.ResetTimer()
for b.Loop() {
sortPermissionsPatched(newPerms, old)
}
}

View file

@ -0,0 +1,10 @@
--- a/backend/jottacloud/jottacloud.go
+++ b/backend/jottacloud/jottacloud.go
@@ -282,7 +282,7 @@ func (f *Fs) Config(ctx context.Context, name string, m configmap.Mapper, config
return nil, fmt.Errorf("failed to register device: %w", err)
}
m.Set(configClientID, deviceRegistration.ClientID)
m.Set(configClientSecret, obscure.MustObscure(deviceRegistration.ClientSecret))
- fs.Debugf(nil, "Got clientID %q and clientSecret %q", deviceRegistration.ClientID, deviceRegistration.ClientSecret)
+ fs.Debugf(nil, "Got clientID %q (clientSecret suppressed)", deviceRegistration.ClientID)
}

View file

@ -0,0 +1,12 @@
--- a/backend/shade/shade.go
+++ b/backend/shade/shade.go
@@ -337,7 +337,7 @@ func (f *Fs) Move(ctx context.Context, src fs.Object, remote string) (fs.Object,
err = o.fs.pacer.Call(func() (bool, error) {
resp, err := f.srv.Call(ctx, &opts)
if err != nil && resp.StatusCode == http.StatusBadRequest {
- fs.Debugf(f, "Bad token from server: %v", token)
+ fs.Debugf(f, "Bad token from server (token suppressed)")
}
return resp != nil && resp.StatusCode == http.StatusTooManyRequests, err