From f58c72ed59b9e360c31adef6814ba35c2ee1b5b5 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Fri, 7 Mar 2025 14:44:29 +0100 Subject: [PATCH 1/5] C#: Add example for local not disposed involving tasks. --- .../NoDisposeCallOnLocalIDisposable.cs | 8 ++++++++ .../NoDisposeCallOnLocalIDisposable.expected | 13 +++++++------ 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/csharp/ql/test/query-tests/API Abuse/NoDisposeCallOnLocalIDisposable/NoDisposeCallOnLocalIDisposable.cs b/csharp/ql/test/query-tests/API Abuse/NoDisposeCallOnLocalIDisposable/NoDisposeCallOnLocalIDisposable.cs index ae9bccf0e6e2..367367dda7e9 100644 --- a/csharp/ql/test/query-tests/API Abuse/NoDisposeCallOnLocalIDisposable/NoDisposeCallOnLocalIDisposable.cs +++ b/csharp/ql/test/query-tests/API Abuse/NoDisposeCallOnLocalIDisposable/NoDisposeCallOnLocalIDisposable.cs @@ -4,6 +4,7 @@ using System.IO.Compression; using System.Xml; using System.Threading; +using System.Threading.Tasks; class Test { @@ -86,6 +87,13 @@ public IDisposable Method() using (XmlReader.Create(source ?? new StringReader("xml"), null)) ; + // GOOD: Flagging these generates to much noise and there is a general + // acceptance that Tasks are not disposed. + // https://devblogs.microsoft.com/pfxteam/do-i-need-to-dispose-of-tasks/ + Task t = new Task(() => { }); + t.Start(); + t.Wait(); + return null; } diff --git a/csharp/ql/test/query-tests/API Abuse/NoDisposeCallOnLocalIDisposable/NoDisposeCallOnLocalIDisposable.expected b/csharp/ql/test/query-tests/API Abuse/NoDisposeCallOnLocalIDisposable/NoDisposeCallOnLocalIDisposable.expected index 1d71aa4af027..195413e43436 100644 --- a/csharp/ql/test/query-tests/API Abuse/NoDisposeCallOnLocalIDisposable/NoDisposeCallOnLocalIDisposable.expected +++ b/csharp/ql/test/query-tests/API Abuse/NoDisposeCallOnLocalIDisposable/NoDisposeCallOnLocalIDisposable.expected @@ -1,7 +1,8 @@ -| NoDisposeCallOnLocalIDisposable.cs:50:19:50:38 | object creation of type Timer | Disposable 'Timer' is created but not disposed. | -| NoDisposeCallOnLocalIDisposable.cs:51:18:51:73 | object creation of type FileStream | Disposable 'FileStream' is created but not disposed. | -| NoDisposeCallOnLocalIDisposable.cs:52:9:52:64 | object creation of type FileStream | Disposable 'FileStream' is created but not disposed. | -| NoDisposeCallOnLocalIDisposable.cs:74:25:74:71 | call to method Create | Disposable 'XmlReader' is created but not disposed. | -| NoDisposeCallOnLocalIDisposable.cs:74:42:74:64 | object creation of type StringReader | Disposable 'StringReader' is created but not disposed. | -| NoDisposeCallOnLocalIDisposable.cs:79:38:79:67 | object creation of type StreamWriter | Disposable 'StreamWriter' is created but not disposed. | +| NoDisposeCallOnLocalIDisposable.cs:51:19:51:38 | object creation of type Timer | Disposable 'Timer' is created but not disposed. | +| NoDisposeCallOnLocalIDisposable.cs:52:18:52:73 | object creation of type FileStream | Disposable 'FileStream' is created but not disposed. | +| NoDisposeCallOnLocalIDisposable.cs:53:9:53:64 | object creation of type FileStream | Disposable 'FileStream' is created but not disposed. | +| NoDisposeCallOnLocalIDisposable.cs:75:25:75:71 | call to method Create | Disposable 'XmlReader' is created but not disposed. | +| NoDisposeCallOnLocalIDisposable.cs:75:42:75:64 | object creation of type StringReader | Disposable 'StringReader' is created but not disposed. | +| NoDisposeCallOnLocalIDisposable.cs:80:38:80:67 | object creation of type StreamWriter | Disposable 'StreamWriter' is created but not disposed. | +| NoDisposeCallOnLocalIDisposable.cs:93:18:93:36 | object creation of type Task | Disposable 'Task' is created but not disposed. | | NoDisposeCallOnLocalIDisposableBad.cs:8:22:8:56 | object creation of type FileStream | Disposable 'FileStream' is created but not disposed. | From 7a99dfaebedd023481aaa75b04abe6d69dd33bd5 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Fri, 7 Mar 2025 15:14:07 +0100 Subject: [PATCH 2/5] C#: Do flag missing Dispose calls on Task and Task<>. --- .../NoDisposeCallOnLocalIDisposable.ql | 27 ++++++++++++++----- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/csharp/ql/src/API Abuse/NoDisposeCallOnLocalIDisposable.ql b/csharp/ql/src/API Abuse/NoDisposeCallOnLocalIDisposable.ql index 3072b154812f..e5826c423427 100644 --- a/csharp/ql/src/API Abuse/NoDisposeCallOnLocalIDisposable.ql +++ b/csharp/ql/src/API Abuse/NoDisposeCallOnLocalIDisposable.ql @@ -16,6 +16,7 @@ import csharp import Dispose import semmle.code.csharp.frameworks.System +import semmle.code.csharp.frameworks.system.threading.Tasks import semmle.code.csharp.commons.Disposal private class ReturnNode extends DataFlow::ExprNode { @@ -24,15 +25,27 @@ private class ReturnNode extends DataFlow::ExprNode { } } +private class Task extends Type { + Task() { + this instanceof SystemThreadingTasksTaskClass or + this instanceof SystemThreadingTasksTaskTClass + } +} + module DisposeCallOnLocalIDisposableConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node node) { - node.asExpr() = - any(LocalScopeDisposableCreation disposable | - // Only care about library types - user types often have spurious IDisposable declarations - disposable.getType().fromLibrary() and - // WebControls are usually disposed automatically - not disposable.getType() instanceof WebControl - ) + exists(LocalScopeDisposableCreation disposable, Type t | + node.asExpr() = disposable and + t = disposable.getType() + | + // Only care about library types - user types often have spurious IDisposable declarations + t.fromLibrary() and + // WebControls are usually disposed automatically + not t instanceof WebControl and + // It is typically not nessesary to dispose tasks + // https://devblogs.microsoft.com/pfxteam/do-i-need-to-dispose-of-tasks/ + not t instanceof Task + ) } predicate isSink(DataFlow::Node node) { From 3f8679a099ca6b53ab29fb96cfe22bfaacda26aa Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Fri, 7 Mar 2025 16:00:28 +0100 Subject: [PATCH 3/5] C#: Update test expected output. --- .../NoDisposeCallOnLocalIDisposable.expected | 1 - 1 file changed, 1 deletion(-) diff --git a/csharp/ql/test/query-tests/API Abuse/NoDisposeCallOnLocalIDisposable/NoDisposeCallOnLocalIDisposable.expected b/csharp/ql/test/query-tests/API Abuse/NoDisposeCallOnLocalIDisposable/NoDisposeCallOnLocalIDisposable.expected index 195413e43436..1a72517edb8b 100644 --- a/csharp/ql/test/query-tests/API Abuse/NoDisposeCallOnLocalIDisposable/NoDisposeCallOnLocalIDisposable.expected +++ b/csharp/ql/test/query-tests/API Abuse/NoDisposeCallOnLocalIDisposable/NoDisposeCallOnLocalIDisposable.expected @@ -4,5 +4,4 @@ | NoDisposeCallOnLocalIDisposable.cs:75:25:75:71 | call to method Create | Disposable 'XmlReader' is created but not disposed. | | NoDisposeCallOnLocalIDisposable.cs:75:42:75:64 | object creation of type StringReader | Disposable 'StringReader' is created but not disposed. | | NoDisposeCallOnLocalIDisposable.cs:80:38:80:67 | object creation of type StreamWriter | Disposable 'StreamWriter' is created but not disposed. | -| NoDisposeCallOnLocalIDisposable.cs:93:18:93:36 | object creation of type Task | Disposable 'Task' is created but not disposed. | | NoDisposeCallOnLocalIDisposableBad.cs:8:22:8:56 | object creation of type FileStream | Disposable 'FileStream' is created but not disposed. | From 13226edbebf03624a41c45f37d2c0f85da061252 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Mon, 10 Mar 2025 10:06:23 +0100 Subject: [PATCH 4/5] C#: Add change-note. --- csharp/ql/src/change-notes/2025-03-10-task-not-disposed.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 csharp/ql/src/change-notes/2025-03-10-task-not-disposed.md diff --git a/csharp/ql/src/change-notes/2025-03-10-task-not-disposed.md b/csharp/ql/src/change-notes/2025-03-10-task-not-disposed.md new file mode 100644 index 000000000000..faf748d873f6 --- /dev/null +++ b/csharp/ql/src/change-notes/2025-03-10-task-not-disposed.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The `cs/local-not-disposed` query no longer flags un-disposed tasks as this is often not needed (explained [here](https://devblogs.microsoft.com/pfxteam/do-i-need-to-dispose-of-tasks/)). From 26f0f7f6da21bfd01a4fdd3c2791ee36d9fbf5af Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Mon, 10 Mar 2025 12:40:25 +0100 Subject: [PATCH 5/5] Update csharp/ql/test/query-tests/API Abuse/NoDisposeCallOnLocalIDisposable/NoDisposeCallOnLocalIDisposable.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .../NoDisposeCallOnLocalIDisposable.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/csharp/ql/test/query-tests/API Abuse/NoDisposeCallOnLocalIDisposable/NoDisposeCallOnLocalIDisposable.cs b/csharp/ql/test/query-tests/API Abuse/NoDisposeCallOnLocalIDisposable/NoDisposeCallOnLocalIDisposable.cs index 367367dda7e9..457c1ae4b40d 100644 --- a/csharp/ql/test/query-tests/API Abuse/NoDisposeCallOnLocalIDisposable/NoDisposeCallOnLocalIDisposable.cs +++ b/csharp/ql/test/query-tests/API Abuse/NoDisposeCallOnLocalIDisposable/NoDisposeCallOnLocalIDisposable.cs @@ -87,7 +87,7 @@ public IDisposable Method() using (XmlReader.Create(source ?? new StringReader("xml"), null)) ; - // GOOD: Flagging these generates to much noise and there is a general + // GOOD: Flagging these generates too much noise and there is a general // acceptance that Tasks are not disposed. // https://devblogs.microsoft.com/pfxteam/do-i-need-to-dispose-of-tasks/ Task t = new Task(() => { });