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(windows): crash on out of bounds for root path cleanup #241

Closed
wants to merge 3 commits into from

Conversation

manifestori
Copy link

@manifestori manifestori commented May 16, 2024

I am using this lib in a tool I've been contributing to.

import builder_common "github.com/spdx/tools-golang/builder"

cwd, err := os.Getwd()
if err != nil {
	return nil, err
}

...
ref := &builder.Config{
		NamespacePrefix: "https://spdx.org/spdxdocs/",
		CreatorType:     "Tool",
		Creator:         "me",
}


builder_common.Build("mypackage", cwd, ref)

On Linux - no issues.
On Windows - I get out of bounds. Also, I tried running tests (without any modifications), but they fail (due to Windows path issues).
I tried my best to test it in the given state.

Also, note that I changed "." to ".\\" .
if a root path has "/" for suffix (ie. my\path\ ) this will not impose a problem ( .\my\file.txt equals '.\my\file.txt ). having only "." is problematic for cases where the root path includes \ and the rel path is just .file.txt and not .\file.txt

Im not a Windows Pro but this seems right

@kzantow
Copy link
Collaborator

kzantow commented May 17, 2024

Hi @manifestori; thanks for the contribution! It looks to me like this library should support Windows for some of this functionality, so I think we should make sure these code paths work by running the tests on Windows, too. I thought I'd give that a try, so I made a PR with a simple workflow to also run the existing tests on Windows. Unfortunately, it looks like there are some definite incompatibilities at least between the tests and Windows. I looked in to this last night a bit, and as expected, this is primarily due to path handling, but the fix in this PR also had some unexpected behavior where tests, at least, got double slashes e.g. .\\something rather than .\something. I don't use this functionality myself, but I'd like to try to make sure it works in expected ways for everyone. I'd love feedback about how to best handle Windows moving forward. I posed a question about the main issues I found when investigating here, if you had any thoughts, please do add them: #117 (comment)

@manifestori
Copy link
Author

Yeah, as I expected .\\someting vs .\something I can make sure to add the correct prefix (. or .\)
they are equivalent, but I see why we would want to differentiate them.

Currently, I had to issue a prompt fix as this is currently broken; I will be using my fork until we resolve this, but I suggest we issue this as 0.5.5

I'll be happy to contribute to the test discussions as well, especially with the up-and-coming spdx3.0 support.

@manifestori
Copy link
Author

@kzantow added trim to double slashes if input ended up having double slash (".\\\\" with escapes).
They are semantically equivalent but syntactically different anyway. wdyt?

@manifestori
Copy link
Author

closing in favor of #242

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