Skip to content

feat: add binary update/upgrade#263

Merged
yanmxa merged 5 commits into
genai-io:mainfrom
hchenxa:hchenxa-0701
Jul 2, 2026
Merged

feat: add binary update/upgrade#263
yanmxa merged 5 commits into
genai-io:mainfrom
hchenxa:hchenxa-0701

Conversation

@hchenxa

@hchenxa hchenxa commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

What & why

#261

Type of change

  • Bug fix
  • New feature
  • Refactor / cleanup
  • Docs
  • Other:

Testing

GOCACHE=/private/tmp/san-go-build-cache go test ./...

Checklist

  • Follows Conventional Commits (feat:, fix:, docs:, …)
  • All commits signed off (git commit -s) — certifies the DCO
  • Tests pass locally
  • Docs updated (if behavior or config changed)
  • Read and follow the Code of Conduct

hchenxa added 2 commits July 1, 2026 17:28
Signed-off-by: hchenxa <hchenxa1986@qq.com>
Signed-off-by: hchenxa <hchenxa1986@qq.com>
@hchenxa hchenxa changed the title add binary update/upgrade feat: add binary update/upgrade Jul 1, 2026
@hchenxa

hchenxa commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

/assign @yanmxa

@yanmxa

yanmxa commented Jul 2, 2026

Copy link
Copy Markdown
Member

Thanks @hchenxa — nice addition, and the test coverage is thorough! 🙏

One thing to fix before merge:

Cross-device rename will fail on many Linux setups. The binary is extracted into os.MkdirTemp("", ...) (i.e. $TMPDIR / /tmp), then installed with os.Rename(binPath, exe). When /tmp is a tmpfs (the systemd default on Fedora/Arch and others) and the binary lives on a different filesystem such as our default ~/.local/bin, os.Rename returns EXDEV ("invalid cross-device link") and the update aborts. Note install.sh gets away with the same move only because shell mv falls back to copy — os.Rename does not.

Suggested fix: create the temp dir next to the target binary, e.g. os.MkdirTemp(filepath.Dir(exe), ".san-update-*"), so the final rename stays on one filesystem (with a copy fallback on EXDEV if you want to be extra safe).

Minor: releases ship Windows as .zip, but the asset name is hard-coded to .tar.gz, so san update won't find a Windows asset — worth either handling .zip or explicitly noting Windows isn't supported yet.

hchenxa and others added 2 commits July 2, 2026 15:33
Signed-off-by: hchenxa <hchenxa1986@qq.com>
Integration tests under tests/integration/ assume a Unix
environment (shell, ./san paths, etc.) and are not safe
to run on windows-latest.

Switch smoke-test zip step from bash (zip/unzip) to
PowerShell native Compress-Archive / Expand-Archive.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: hchenxa <hchenxa1986@qq.com>
@hchenxa

hchenxa commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

@yanmxa this is a good suggestion. and I also add the windows ci testing, please take a look at again

extractTarGz joined the archive entry name onto destDir without
checking that the result stayed inside destDir, unlike extractZip
which already guards against zip slip. Add the same check so a
crafted tarball cannot write outside the temp dir.

Signed-off-by: Meng Yan <yanmxa@gmail.com>

@yanmxa yanmxa left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM — the EXDEV cross-device fix (temp dir next to the target binary + copy fallback) and the Windows .zip handling with the new Windows CI job both look good, and the test coverage is thorough.

I pushed one small follow-up commit: extractTarGz was missing the path-traversal guard that extractZip already has, so a crafted tarball entry could write outside the temp dir. Added the same check plus a test. Thanks for the solid work @hchenxa!

@yanmxa yanmxa merged commit f1b203e into genai-io:main Jul 2, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants