diff --git a/corpus/real-world/ado-executereader-leak/after.cs b/corpus/real-world/ado-executereader-leak/after.cs new file mode 100644 index 00000000..1faa3a7b --- /dev/null +++ b/corpus/real-world/ado-executereader-leak/after.cs @@ -0,0 +1,14 @@ +using System.Data.Common; + +// FIX: own the reader for the scope with `using`, so it is disposed on every exit path. +static class AdoReaderLeak +{ + static int Run(DbCommand cmd) + { + using var reader = cmd.ExecuteReader(); // disposed at scope exit -> clean + var n = 0; + while (reader.Read()) + n++; + return n; + } +} diff --git a/corpus/real-world/ado-executereader-leak/before.cs b/corpus/real-world/ado-executereader-leak/before.cs new file mode 100644 index 00000000..d69020ec --- /dev/null +++ b/corpus/real-world/ado-executereader-leak/before.cs @@ -0,0 +1,17 @@ +using System.Data.Common; + +// A DbDataReader returned by DbCommand.ExecuteReader() is a fresh owned IDisposable the caller +// must dispose -- dropping it leaks the reader and holds the underlying server-side cursor open +// until finalization. The command here is a borrowed parameter (the caller owns it), so the ONLY +// leak is `reader`. This is the single most common real-world ADO.NET resource leak. +static class AdoReaderLeak +{ + static int Run(DbCommand cmd) + { + var reader = cmd.ExecuteReader(); // fresh owned DbDataReader -> OWN001 (never disposed) + var n = 0; + while (reader.Read()) + n++; + return n; // BUG: reader never disposed + } +} diff --git a/corpus/real-world/ado-executereader-leak/case.own b/corpus/real-world/ado-executereader-leak/case.own new file mode 100644 index 00000000..1d4ca220 --- /dev/null +++ b/corpus/real-world/ado-executereader-leak/case.own @@ -0,0 +1,18 @@ +// OwnLang model of the canonical ADO.NET reader leak (P1a, ADO.NET tranche). A DbDataReader +// from DbCommand.ExecuteReader() is a fresh owned IDisposable the caller must dispose; here it +// is acquired, read, and never released — the generic OWN001 leak. The command is a borrowed +// parameter and the reader does not escape (only a count is returned), so it stays tracked. +// See notes.md for the recognition rule (return type implements System.Data.IDataReader). +module Corpus +resource Reader { + acquire open + release dispose + kind "disposable" + emit_type "DbDataReader" + emit_acquire "{args}.ExecuteReader()" + emit_release "{0}.Dispose()" +} +fn Run(cmd: int) { + let reader = acquire Reader(cmd); // var reader = cmd.ExecuteReader() + // rows read via reader.Read(); no `release reader;` — never disposed (OWN001) +} diff --git a/corpus/real-world/ado-executereader-leak/expected-diagnostics.txt b/corpus/real-world/ado-executereader-leak/expected-diagnostics.txt new file mode 100644 index 00000000..ed2a1929 --- /dev/null +++ b/corpus/real-world/ado-executereader-leak/expected-diagnostics.txt @@ -0,0 +1 @@ +OWN001 diff --git a/corpus/real-world/ado-executereader-leak/notes.md b/corpus/real-world/ado-executereader-leak/notes.md new file mode 100644 index 00000000..15759b48 --- /dev/null +++ b/corpus/real-world/ado-executereader-leak/notes.md @@ -0,0 +1,18 @@ +# ado-executereader-leak + +`DbCommand.ExecuteReader()` returns a fresh **owned** `DbDataReader` the caller must +dispose. Dropping it leaks the reader and keeps the server-side cursor/connection busy +until finalization — the single most common real-world ADO.NET resource leak. + +- **before.cs** — `var reader = cmd.ExecuteReader();` used and never disposed → `OWN001`. + The command is a borrowed parameter, so the only leak is the reader. +- **after.cs** — `using var reader = …` disposes it on every path → clean. + +Recognised by the extractor's `IsOwningFactory` (P1a, ADO.NET tranche): matched by method +name + **both** the receiver and the return type implementing the `System.Data` contract +interfaces — the receiver an `IDbCommand` and the return an `IDataReader` — so it covers every +provider (`SqlDataReader`, `NpgsqlDataReader`, …), the abstract `DbDataReader`, and the +interface, while a non-ADO helper that merely exposes an `ExecuteReader` returning a borrowed +reader is not mistaken for an owned factory. Sibling members `CreateCommand` +(`IDbConnection` → `IDbCommand`) and `BeginTransaction` (`IDbConnection` → `IDbTransaction`) +are recognised the same way. diff --git a/frontend/roslyn/OwnSharp.Extractor/Program.cs b/frontend/roslyn/OwnSharp.Extractor/Program.cs index ba602b2c..f353ad0d 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -2299,9 +2299,41 @@ static bool IsOwningFactory(ExpressionSyntax? e, SemanticModel model) // overloads have no disposable arg and still resolve. (precision over recall.) && !AnyDisposableArgument(i, model)) return true; + // ADO.NET owned-returning members — the canonical real-world disposable leak. These are + // INSTANCE methods on a connection/command, but the acquire path (op="acquire") does not care + // static-vs-instance, and the receiver is not an argument so it is never dropped. Provider + // types vary (SqlCommand / NpgsqlCommand / SqliteCommand / ...), so match by method name + the + // RESOLVED types implementing the System.Data contract interfaces, which covers every provider, + // the abstract base (DbDataReader/DbCommand/DbTransaction), and the interface itself. BOTH the + // RECEIVER and the RETURN are pinned (like every other factory branch verifies its declaring + // type), so a non-ADO helper that merely exposes an `ExecuteReader` returning a borrowed/cached + // IDataReader is NOT mistaken for an owned factory (Codex): + // * IDbCommand.ExecuteReader() -> a DbDataReader the caller must dispose (frees the cursor) + // * IDbConnection.CreateCommand() -> a DbCommand the caller must dispose + // * IDbConnection.BeginTransaction() -> a DbTransaction the caller must dispose + // Arguments are non-disposable (CommandBehavior / IsolationLevel enums); the guard keeps any + // odd overload from dropping a disposable input. + if (!AnyDisposableArgument(i, model) + && ((sym.Name == "ExecuteReader" + && ImplementsSystemDataInterface(sym.ContainingType, "IDbCommand") + && ImplementsSystemDataInterface(sym.ReturnType, "IDataReader")) + || (sym.Name == "CreateCommand" + && ImplementsSystemDataInterface(sym.ContainingType, "IDbConnection") + && ImplementsSystemDataInterface(sym.ReturnType, "IDbCommand")) + || (sym.Name == "BeginTransaction" + && ImplementsSystemDataInterface(sym.ContainingType, "IDbConnection") + && ImplementsSystemDataInterface(sym.ReturnType, "IDbTransaction")))) + return true; return false; } +// True if `t` IS, or implements, the named `System.Data` interface (e.g. IDataReader). Covers a +// provider's concrete type (SqlDataReader), the abstract base (DbDataReader), and the interface. +static bool ImplementsSystemDataInterface(ITypeSymbol? t, string iface) => + t is not null + && ((t.Name == iface && IsInNamespace(t as INamedTypeSymbol, "System", "Data")) + || t.AllInterfaces.Any(i => i.Name == iface && IsInNamespace(i, "System", "Data"))); + // True if any argument to the call resolves to a type implementing IDisposable — a disposable // the callee might NOT take ownership of, so an enclosing owned-factory claim must be declined // rather than silently drop that argument from leak tracking.