From 9517a5759abb0644f96c845e4665d610fc62c654 Mon Sep 17 00:00:00 2001
From: Dominik Csapak <d.csapak@proxmox.com>
Date: Wed, 14 Jul 2021 09:30:26 +0200
Subject: [PATCH] fix #3526: correctly filter tasks with 'since' and 'until'

The previous assumption was that the Tasks returned by the Iterator are
sorted by the starttime, but that is not actually the case, and
could never have been, since we append the tasks into the log when
they are finished (not started) and running tasks are always iterated
first.

To correctly filter (and simplify the the api call) we forgo the
combinators, and use a for loop instead. This way we only have to do
the since/until checks only once per Task, but have to do the
start/limit counting ourselves.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/api2/node/tasks.rs | 76 ++++++++++++++++++++++++------------------
 1 file changed, 43 insertions(+), 33 deletions(-)

diff --git a/src/api2/node/tasks.rs b/src/api2/node/tasks.rs
index 34e71af1..1602de6f 100644
--- a/src/api2/node/tasks.rs
+++ b/src/api2/node/tasks.rs
@@ -451,63 +451,73 @@ pub fn list_tasks(
     let list = TaskListInfoIterator::new(running)?;
     let limit = if limit > 0 { limit as usize } else { usize::MAX };
 
-    let result: Vec<TaskListItem> = list
-        .skip_while(|info| {
-            match (info, until) {
-                (Ok(info), Some(until)) => info.upid.starttime > until,
-                (Ok(_), None) => false,
-                (Err(_), _) => false,
-            }
-        })
-        .take_while(|info| {
-            match (info, since) {
-                (Ok(info), Some(since)) => info.upid.starttime > since,
-                (Ok(_), None) => true,
-                (Err(_), _) => false,
-            }
-        })
-        .filter_map(|info| {
+    let mut skipped = 0;
+    let mut result: Vec<TaskListItem> = Vec::new();
+
+    for info in list {
         let info = match info {
             Ok(info) => info,
-            Err(_) => return None,
+            Err(_) => break,
         };
 
+        if let Some(until) = until {
+            if info.upid.starttime > until {
+                continue;
+            }
+        }
+
+        if let Some(since) = since {
+            if let Some(ref state) = info.state {
+                if state.endtime() < since {
+                    // we reached the tasks that ended before our 'since'
+                    // so we can stop iterating
+                    break;
+                }
+            }
+            if info.upid.starttime < since {
+                continue;
+            }
+        }
+
         if !list_all && check_task_access(&auth_id, &info.upid).is_err() {
-            return None;
+            continue;
         }
 
         if let Some(needle) = &userfilter {
-            if !info.upid.auth_id.to_string().contains(needle) { return None; }
+            if !info.upid.auth_id.to_string().contains(needle) { continue; }
         }
 
         if let Some(store) = store {
-            if !check_job_store(&info.upid, store) {
-                return None;
-            }
+            if !check_job_store(&info.upid, store) { continue; }
         }
 
         if let Some(typefilter) = &typefilter {
-            if !info.upid.worker_type.contains(typefilter) {
-                return None;
-            }
+            if !info.upid.worker_type.contains(typefilter) { continue; }
         }
 
         match (&info.state, &statusfilter) {
-            (Some(_), _) if running => return None,
-            (Some(crate::server::TaskState::OK { .. }), _) if errors => return None,
+            (Some(_), _) if running => continue,
+            (Some(crate::server::TaskState::OK { .. }), _) if errors => continue,
             (Some(state), Some(filters)) => {
                 if !filters.contains(&state.tasktype()) {
-                    return None;
+                    continue;
                 }
             },
-            (None, Some(_)) => return None,
+            (None, Some(_)) => continue,
             _ => {},
         }
 
-        Some(info.into())
-    }).skip(start as usize)
-        .take(limit)
-        .collect();
+        if skipped < start as usize {
+            skipped += 1;
+            continue;
+        }
+
+        result.push(info.into());
+
+        if result.len() >= limit {
+            break;
+        }
+    }
 
     let mut count = result.len() + start as usize;
     if !result.is_empty() && result.len() >= limit { // we have a 'virtual' entry as long as we have any new