Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Resolve panic for long pathnames in npm tarballs #32

Merged
merged 9 commits into from
Feb 29, 2024
Merged

Conversation

ry
Copy link
Contributor

@ry ry commented Feb 28, 2024

This fix addresses a panic during npm tarball creation caused by not accounting for path length limits. Specifically, we already required the total path length must be under 160 characters to be compatible with Windows, but we need to limit it further to 155 to accommodate tarball constraints. Additionally, the final path component is restricted to under 100 characters, which we've reduced to 95 to permit extension modifications (e.g., adding '.d.ts').

Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

Rather than doing this, can you just add this as a validation in PackagePath::new (but limit it to 95 so we don't go over when changing file extensions)? That way files with long names can not even get into this erroring situation.

@arnauorriols
Copy link

deno vendor tends to generate very long paths, potentially surpasing the limit (like in the case of deployctl that triggered this issue). Maybe we should warn about it and suggest the workaround in the deno vendor section of the x to jsr migration docs.

Copy link
Contributor Author

@ry ry left a comment

Choose a reason for hiding this comment

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

Not super happy with the current solution. don't land

@ry
Copy link
Contributor Author

ry commented Feb 29, 2024

This is ready for review now.

The patch fixes the problem, but is not ideal. What we really need is a way to propagate publish errors from deep inside the code base. Ideally there is a system to collect these and even return more than one at a time.

For now, it seems best to handle almost all cases by changing the limit to 100 characters - even tho that's not exactly true - still need room for "package/" and appending ".d" to file names. That accounts for 90 characters, but there might be other situations. Therefore let's just tell people it's 100 improve the error messages and revisit this later.

People with paths between ~90 and 100 bytes will get an error with the path in it, but it won't be the right error code.

api/src/npm/tarball.rs Outdated Show resolved Hide resolved
@lucacasonato
Copy link
Member

@ry So I didn't mean to change 160 -> 100. There are two limits at play here coming from tar:

  • 255 max bytes for the entire (relative) path string (we cap this length to 160 in PackagePath). 160 < 255, so this limit is fine.
  • 100 max bytes for the name in the path string. In the path ./foo/bar/bazzz the name is bazzz, so the name length is 5 (not 15).

You can see the code for this here: https://docs.rs/tar/latest/src/tar/header.rs.html#975-1026

So instead of checking that total length <= 160 (very restrictive), check that the total length is < 100, and that the final component of the path is less than 95.

Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

LGTM

@ry ry changed the title fix: segfault on long paths fix: Resolve panic for long pathnames in npm tarballs Feb 29, 2024
@ry ry added this pull request to the merge queue Feb 29, 2024
Merged via the queue into main with commit 7ddf8c9 Feb 29, 2024
5 checks passed
@ry ry deleted the fix-segfault branch February 29, 2024 11:00
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.

3 participants