Skip to content

[ffigen] Fix potential null access for _tmpDir when creating temporary directories#3029

Open
Gurleen-kansray wants to merge 6 commits intodart-lang:mainfrom
Gurleen-kansray:fix-tmpdir-null
Open

[ffigen] Fix potential null access for _tmpDir when creating temporary directories#3029
Gurleen-kansray wants to merge 6 commits intodart-lang:mainfrom
Gurleen-kansray:fix-tmpdir-null

Conversation

@Gurleen-kansray
Copy link
Contributor

@Gurleen-kansray Gurleen-kansray commented Jan 29, 2026

This PR fixes a potential null access issue for _tmpDir when creating a temporary directory. The assignment now safely uses the null-aware operator to ensure _tmpDir is initialized properly:

_tmpDir ??= Directory.systemTemp.createTempSync('temp dir ');

Verified with scope_test.dart, which directly tests this functionality.

The change is localized, so it should not affect unrelated functionality. This ensures safer initialization of _tmpDir and prevents potential runtime null errors.

[x] I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Closes #3026

@liamappelbe
Copy link
Contributor

PR description is inaccurate. The code was already null safe, you're just changing the name of the temp directory.

Setting that aside, I'd prefer if this change only affected the tests. Maybe change it from a getter to a function with an optional prefix param, and pass a path with a space in the tests.

But now that I'm looking at this code, I notice an issue with it (not related to your change, but it'd be nice if you could fix this at the same time). I recently tried to eliminate all the global variables from FFIgen, but it seems I missed one. _tmpDir should be deleted and moved to the Context object. The issue with having global variables like this is that if we want to do multiple concurrent FFIgen runs, they'll conflict with each other.

Moving tmpDir to Context will also make it easier to make the test paths contain spaces. Make the constructor of Context optionally take a tmpDir, and fall back to the existing logic if the user doesn't pass tmpDir. Then you can just look for all the places Context is constructed in the test dirs (ie, search test/ for Context( in your IDE), and pass a directory name with a space in it. I think most of the tests use a util to construct the context, so there shouldn't be many places to update.

@liamappelbe liamappelbe self-requested a review January 29, 2026 23:54
@Gurleen-kansray
Copy link
Contributor Author

Thanks for the feedback @liamappelbe ..you’re right, my PR description was inaccurate. The code was already null-safe, and the change effectively only altered the temp directory name.
Before reworking this, I wanted to clarify: is there an existing execution/context object in ffigen that you’d like the temp directory to live on, or would this involve introducing a new context abstraction and threading it through the call sites?

I want to make sure I’m aligning with the intended direction before attempting a larger refactor.

@liamappelbe
Copy link
Contributor

Before reworking this, I wanted to clarify: is there an existing execution/context object in ffigen that you’d like the temp directory to live on, or would this involve introducing a new context abstraction and threading it through the call sites?

There's already a Context object plumbed through most of ffigen: https://github.com/dart-lang/native/blob/main/pkgs/ffigen/lib/src/context.dart#L18

@Gurleen-kansray
Copy link
Contributor Author

Hi @liamappelbe, I updated _matchFileWithExpected to require a Context so it can use tmpDir from the context, avoiding conflicts in concurrent FFIgen runs. This affects all tests calling it. Should I go ahead and update the tests to pass Context, or keep backward compatibility for now?

@liamappelbe
Copy link
Contributor

Hi @liamappelbe, I updated _matchFileWithExpected to require a Context so it can use tmpDir from the context, avoiding conflicts in concurrent FFIgen runs. This affects all tests calling it. Should I go ahead and update the tests to pass Context, or keep backward compatibility for now?

There's no need to maintain backwards compatibility in tests. Definitely update the tests.

@github-actions
Copy link

github-actions bot commented Feb 4, 2026

PR Health

License Headers ✔️
// Copyright (c) 2026, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
Files
no missing headers

All source files should start with a license header.

Unrelated files missing license headers
Files
pkgs/hooks_runner/test_data/download_assets/hook/build.dart
pkgs/jni/test/debug_release_test.dart
pkgs/objective_c/example/command_line/lib/main.dart
pkgs/objective_c/lib/src/ns_input_stream.dart

This check can be disabled by tagging the PR with skip-license-check.

API leaks ✔️

The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.

Package Leaked API symbol Leaking sources

This check can be disabled by tagging the PR with skip-leaking-check.

Breaking changes ✔️
Package Change Current Version New Version Needed Version Looking good?

This check can be disabled by tagging the PR with skip-breaking-check.

Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

This check can be disabled by tagging the PR with skip-changelog-check.

@coveralls
Copy link

coveralls commented Feb 4, 2026

Coverage Status

coverage: 81.802%. remained the same
when pulling 2128376 on Gurleen-kansray:fix-tmpdir-null
into 39e3e71 on dart-lang:main.

// Initialize compiler options
compilerOpts = config.headers.compilerOptions ?? defaultCompilerOpts(logger);

objCBuiltInFunctions = ObjCBuiltInFunctions(
Copy link
Contributor

Choose a reason for hiding this comment

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

What's with all these unrelated diffs? You've removed some important stuff.

Copy link
Contributor

Choose a reason for hiding this comment

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

You've still got this global variable. The point was to move this into the context.

@Gurleen-kansray
Copy link
Contributor Author

Thanks, that makes sense.

You’re right ......I added Context to _matchFileWithExpected but didn’t actually use it yet. I’ll update it to read the temp directory from context.tmpDir.

The unrelated diffs weren’t intentional; I’ll revert those and keep the change focused.

I also see the remaining global _tmpDir.....I’ll remove it and move all temp dir handling into Context as suggested.

I’ll update the PR accordingly.

@Gurleen-kansray
Copy link
Contributor Author

Following the earlier review, I’ve updated _matchFileWithExpected to use context.tmpDir and removed the remaining global _tmpDir. Please let me know if any further adjustments are needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[all packages] Beef up testing by making all temp directories always have spaces

3 participants