Skip to content

Fix: address multiple minor bugs found during code audit#492

Open
vinayakjeet wants to merge 1 commit intourunc-dev:mainfrom
vinayakjeet:fix/audit-bugs
Open

Fix: address multiple minor bugs found during code audit#492
vinayakjeet wants to merge 1 commit intourunc-dev:mainfrom
vinayakjeet:fix/audit-bugs

Conversation

@vinayakjeet
Copy link
Contributor

@vinayakjeet vinayakjeet commented Feb 17, 2026

Description

Refactor error handling and nil-safety: use annotHypervisor in isRunning, remove nil error wrapping, fix consoleFile nil-deref, use errors.Is for sentinel checks, and propagate errors from getInitPid and deleteAllTCFilters.

Related issues

None

How was this tested?

lint and build

LLM usage

None

Checklist

  • I have read the contribution guide.
  • The linter passes locally (make lint).
  • The e2e tests of at least one tool pass locally (make test_ctr, make test_nerdctl, make test_docker, make test_crictl).
  • If LLMs were used: I have read the llm policy.

@netlify
Copy link

netlify bot commented Feb 17, 2026

Deploy Preview for urunc ready!

Name Link
🔨 Latest commit 49a8488
🔍 Latest deploy log https://app.netlify.com/projects/urunc/deploys/6995d98b9c06b00008f8b4ab
😎 Deploy Preview https://deploy-preview-492--urunc.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@vinayakjeet
Copy link
Contributor Author

hi @cmainas can you please review the changes .

@vinayakjeet vinayakjeet changed the title Fix: address multiple bugs found during code audit Fix: address multiple minor bugs found during code audit Feb 17, 2026
Copy link
Contributor

@IrvingMg IrvingMg left a comment

Choose a reason for hiding this comment

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

Some changes need to be reviewed, as they’re breaking unit tests and may not be actual bugs. You can run make unittest to verify that they pass.

@vinayakjeet vinayakjeet marked this pull request as draft February 18, 2026 07:49
@vinayakjeet vinayakjeet marked this pull request as ready for review February 18, 2026 07:49
@vinayakjeet vinayakjeet marked this pull request as draft February 18, 2026 07:49
@cmainas
Copy link
Contributor

cmainas commented Feb 18, 2026

Changes look ok, but just a question. Where does "consoleFile nil-deref" take place?

@vinayakjeet
Copy link
Contributor Author

@cmainas
The nil-deref is in pkg/unikontainers/rootfs.go, in prepareMonRootfs()
in original code did os.Create() with the guard if err != nil && !os.IsExist(err) — so if the file already existed, the error was skipped but consoleFile was still remains nil so consoleFile.Chmod() on the next line would panic.

@cmainas
Copy link
Contributor

cmainas commented Feb 18, 2026

@cmainas The nil-deref is in pkg/unikontainers/rootfs.go, in prepareMonRootfs() in original code did os.Create() with the guard if err != nil && !os.IsExist(err) — so if the file already existed, the error was skipped but consoleFile was still remains nil so consoleFile.Chmod() on the next line would panic.

Yes, that is correct.

@vinayakjeet
Copy link
Contributor Author

Hi @cmainas — I’ve rebased the PR onto the latest main. It should be good to go now 👍

@vinayakjeet vinayakjeet marked this pull request as ready for review February 18, 2026 14:25
@cmainas
Copy link
Contributor

cmainas commented Feb 18, 2026

Thank you @vinayakjeet , please rebase once more to be on the safe side.

@vinayakjeet
Copy link
Contributor Author

@cmainas rebased and updated — it's good to go now. Thanks

- Use annotHypervisor instead of annotType in isRunning()

- Remove nil error wrapping in run.go

- Fix potential nil-deref on consoleFile in rootfs.go

- Use errors.Is() for sentinel error comparison

- Propagate errors in getInitPid instead of swallowing

- Propagate error in deleteAllTCFilters

Signed-off-by: vinayakjeet <vinayakjeetog@gmail.com>
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.

3 participants

Comments