Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 36 additions & 1 deletion frontend/roslyn/OwnSharp.Extractor/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2260,9 +2260,15 @@
// IncrementalHash.CreateHash(), ... (guarded by static + Create-prefixed + the RESULT
// implementing IDisposable + the crypto namespace, so an instance `CreateEncryptor()` or
// a non-IDisposable `CreateFromName()` is never mistaken for one).
// * P1a stdlib pack: System.Xml `XmlReader.Create`/`XmlWriter.Create` -> a fresh owned
// reader/writer; System.Text.Json `JsonDocument.Parse` -> a JsonDocument that pools memory
// and must be disposed. Gated on the RESULT implementing IDisposable, so a Task-returning
// `JsonDocument.ParseAsync` is excluded.
// Curated + symbol-resolved, so a borrowed/cached disposable handed back by some other API is
// never mistaken for an owned acquire (precision over recall — the set grows only as
// ownership is certain).
// ownership is certain). Kept in lockstep with the bridge table `_BCL_FRESH_BY_NS`
// (ownlang/ownir.py): the extractor decides whether to EMIT the factory call fact, the bridge
// recognises the callee name as `fresh`.
static bool IsOwningFactory(ExpressionSyntax? e, SemanticModel model)
{
if (e is not InvocationExpressionSyntax i
Expand All @@ -2278,6 +2284,35 @@
&& ImplementsIDisposable(sym.ReturnType)
&& IsInNamespace(sym.ContainingType, "System", "Security", "Cryptography"))
return true;
if (sym.IsStatic && ImplementsIDisposable(sym.ReturnType)
&& ((sym.Name == "Create"
&& sym.ContainingType is { Name: "XmlReader" or "XmlWriter" } xt
&& IsInNamespace(xt, "System", "Xml"))
Comment on lines +2288 to +2290

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve ownership of XmlReader input streams

Please don't classify all XmlReader.Create/XmlWriter.Create overloads as pure factories without also handling their disposable inputs. The extractor's escape pass removes any candidate disposable passed as an argument, and this factory branch later emits only an acquire for the reader/writer; in var fs = File.OpenRead(p); var xr = XmlReader.Create(fs); xr.Dispose();, fs is dropped from tracking even though XmlReaderSettings.CloseInput defaults to false, so the caller-owned stream leak is suppressed. Either restrict the matched overloads or model these inputs as borrowed/non-escaping.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — fixed in 83edff6. You're right that the Stream/TextReader/TextWriter overloads don't own their input (CloseInput/CloseOutput default false, and JsonDocument.Parse never closes its stream), so classifying them as pure factories let the escape pass drop the caller-owned input and suppress its leak.

Went with precision over recall (the project's stated floor): the new factory branch now declines the claim whenever any argument resolves to an IDisposable (!AnyDisposableArgument). So XmlReader.Create(fs) / JsonDocument.Parse(stream) are no longer treated as factories — fs stays tracked and its leak is reported — while the common no-disposable-arg overloads (XmlReader.Create("file.xml"), JsonDocument.Parse(jsonString)) still resolve and catch a dropped reader/document. Modelling the input as borrowed-non-escaping (your option b) would keep recall on those overloads too, but it's a deeper escape-pass change; I took the conservative narrowing for now. No dotnet locally, so CI (golden C# / leak extractor / corpus benchmark) verifies the build + specificity.


Generated by Claude Code

|| (sym.Name == "Parse"
&& sym.ContainingType is { Name: "JsonDocument" } jt
&& IsInNamespace(jt, "System", "Text", "Json")))
// ...but ONLY the overloads that take no disposable input. The Stream/TextReader/
// TextWriter overloads do NOT own that input (XmlReaderSettings.CloseInput /
// XmlWriterSettings.CloseOutput default false, JsonDocument.Parse never closes its
// stream), so claiming a pure factory there would let the escape pass drop the
// caller-owned input and suppress its leak (Codex). The common string/URI/path
// overloads have no disposable arg and still resolve. (precision over recall.)
&& !AnyDisposableArgument(i, model))
return true;
return false;
}

// 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.
static bool AnyDisposableArgument(InvocationExpressionSyntax inv, SemanticModel model)
{
foreach (var arg in inv.ArgumentList.Arguments)
{
var t = model.GetTypeInfo(arg.Expression).Type;
if (t is not null && ImplementsIDisposable(t))
return true;
}
return false;
}

Expand Down Expand Up @@ -2954,7 +2989,7 @@
.Split(Path.PathSeparator, StringSplitOptions.RemoveEmptyEntries)
.Where(p => p.EndsWith(".dll", StringComparison.OrdinalIgnoreCase))
.ToList();
var refNames = new HashSet<string>(tpa.Select(Path.GetFileName), StringComparer.OrdinalIgnoreCase);

Check warning on line 2992 in frontend/roslyn/OwnSharp.Extractor/Program.cs

View workflow job for this annotation

GitHub Actions / own-check SARIF -> GitHub code scanning (dog-food)

Argument of type 'IEnumerable<string?>' cannot be used for parameter 'collection' of type 'IEnumerable<string>' in 'HashSet<string>.HashSet(IEnumerable<string> collection, IEqualityComparer<string>? comparer)' due to differences in the nullability of reference types.

Check warning on line 2992 in frontend/roslyn/OwnSharp.Extractor/Program.cs

View workflow job for this annotation

GitHub Actions / own-check repo scan (github + msbuild) + composite action

Argument of type 'IEnumerable<string?>' cannot be used for parameter 'collection' of type 'IEnumerable<string>' in 'HashSet<string>.HashSet(IEnumerable<string> collection, IEqualityComparer<string>? comparer)' due to differences in the nullability of reference types.

Check warning on line 2992 in frontend/roslyn/OwnSharp.Extractor/Program.cs

View workflow job for this annotation

GitHub Actions / P-014 Tier B — external reference resolution (--ref-dir)

Argument of type 'IEnumerable<string?>' cannot be used for parameter 'collection' of type 'IEnumerable<string>' in 'HashSet<string>.HashSet(IEnumerable<string> collection, IEqualityComparer<string>? comparer)' due to differences in the nullability of reference types.

Check warning on line 2992 in frontend/roslyn/OwnSharp.Extractor/Program.cs

View workflow job for this annotation

GitHub Actions / own-check SARIF -> GitHub code scanning (dog-food)

Argument of type 'IEnumerable<string?>' cannot be used for parameter 'collection' of type 'IEnumerable<string>' in 'HashSet<string>.HashSet(IEnumerable<string> collection, IEqualityComparer<string>? comparer)' due to differences in the nullability of reference types.

Check warning on line 2992 in frontend/roslyn/OwnSharp.Extractor/Program.cs

View workflow job for this annotation

GitHub Actions / P-014 Tier B — external reference resolution (--ref-dir)

Argument of type 'IEnumerable<string?>' cannot be used for parameter 'collection' of type 'IEnumerable<string>' in 'HashSet<string>.HashSet(IEnumerable<string> collection, IEqualityComparer<string>? comparer)' due to differences in the nullability of reference types.

Check warning on line 2992 in frontend/roslyn/OwnSharp.Extractor/Program.cs

View workflow job for this annotation

GitHub Actions / own-check repo scan (github + msbuild) + composite action

Argument of type 'IEnumerable<string?>' cannot be used for parameter 'collection' of type 'IEnumerable<string>' in 'HashSet<string>.HashSet(IEnumerable<string> collection, IEqualityComparer<string>? comparer)' due to differences in the nullability of reference types.

Check warning on line 2992 in frontend/roslyn/OwnSharp.Extractor/Program.cs

View workflow job for this annotation

GitHub Actions / C# leak extractor (Roslyn) -> OwnIR -> core

Argument of type 'IEnumerable<string?>' cannot be used for parameter 'collection' of type 'IEnumerable<string>' in 'HashSet<string>.HashSet(IEnumerable<string> collection, IEqualityComparer<string>? comparer)' due to differences in the nullability of reference types.

Check warning on line 2992 in frontend/roslyn/OwnSharp.Extractor/Program.cs

View workflow job for this annotation

GitHub Actions / C# leak extractor (Roslyn) -> OwnIR -> core

Argument of type 'IEnumerable<string?>' cannot be used for parameter 'collection' of type 'IEnumerable<string>' in 'HashSet<string>.HashSet(IEnumerable<string> collection, IEqualityComparer<string>? comparer)' due to differences in the nullability of reference types.
var references = tpa.Select(p => (MetadataReference)MetadataReference.CreateFromFile(p)).ToList();
// P-004 WPF profile: widen the reference set with assemblies named by the
// OWN_EXTRA_REF_DIRS env var (colon-separated dirs) — e.g. the WindowsDesktop ref
Expand Down
11 changes: 11 additions & 0 deletions ownlang/ownir.py
Original file line number Diff line number Diff line change
Expand Up @@ -1185,6 +1185,17 @@ def _infer_return_skeleton(nodes: Any, param_names: set[str],
"SHA1.Create", "SHA256.Create", "SHA384.Create", "SHA512.Create", "MD5.Create",
"Aes.Create", "RSA.Create", "ECDsa.Create",
),
# P1a (stdlib contract pack): more well-known static factories whose result the caller
# OWNS and must dispose. `XmlReader`/`XmlWriter.Create` return a fresh reader/writer; a
# `JsonDocument` from `Parse` pools memory and must be disposed (a `var doc =
# JsonDocument.Parse(json)` that drops `doc` is a common real leak). Kept in lockstep with
# the extractor's `IsOwningFactory` (frontend/roslyn/.../Program.cs).
"System.Xml": (
"XmlReader.Create", "XmlWriter.Create",
),
"System.Text.Json": (
"JsonDocument.Parse",
),
}
_BCL_FRESH_FACTORIES = frozenset(e for es in _BCL_FRESH_BY_NS.values() for e in es)
# the fully-qualified identities (each under its real namespace), accepted beside the bare forms.
Expand Down
19 changes: 19 additions & 0 deletions tests/test_ownir.py
Original file line number Diff line number Diff line change
Expand Up @@ -1691,6 +1691,25 @@ def _bcl(body: list) -> list:
if _bcl([{"op": "call", "callee": "global::MyCompany.File.OpenRead", "args": ["p"],
"result": "s", "line": 4}]):
fails.append("Tier B precision: `global::`-qualified non-System.IO must NOT match")
# P1a (stdlib pack): more curated owned-returning factories. A dropped XmlReader/XmlWriter/
# JsonDocument result leaks at the factory call (OWN001), the same producer-side contract as
# File.Open* — both the bare `Type.Method` and the namespace-qualified identity resolve.
for fresh_callee, ln in (("XmlReader.Create", 5),
("System.Xml.XmlReader.Create", 6),
("XmlWriter.Create", 7),
("System.Xml.XmlWriter.Create", 8),
("JsonDocument.Parse", 9),
("System.Text.Json.JsonDocument.Parse", 10)):
checks += 1
leak = [(x.code, x.line) for x in _bcl(
[{"op": "call", "callee": fresh_callee, "args": ["a"], "result": "s", "line": ln}])]
if leak != [("OWN001", ln)]:
fails.append(f"P1a: a leaked `{fresh_callee}` result must be OWN001@{ln}, got {leak}")
# disposing the P1a factory result is clean (no false leak), proving it is a real acquire.
checks += 1
if _bcl([{"op": "call", "callee": "XmlReader.Create", "args": ["a"], "result": "s", "line": 5},
{"op": "release", "var": "s", "line": 6}]):
fails.append("P1a: a disposed XmlReader.Create result must be clean (silent)")
checks += 1
# OVERRIDE (Codex): a first-party summary is authoritative — a first-party `File.OpenRead`
# that returns its parameter is NOT fresh, so a caller dropping its result is clean; the
Expand Down
Loading