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

bug: IsParent doesn't handle the case where paths are empty properly #1116

Open
HarikrishnanBalagopal opened this issue Dec 8, 2023 · 7 comments
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@HarikrishnanBalagopal
Copy link
Contributor

HarikrishnanBalagopal commented Dec 8, 2023

Bug

Overview

func IsParent(child, parent string) bool {

When path is relative it joins with the current working.
In WASM the current working directory is / and we hit this if condition

move2kube/common/utils.go

Lines 1088 to 1090 in e88cece

if parent == "/" {
return true
}

so it returns true.

And because IsParent returns true, we try to make a path relative to an empty string '' and hit this error

rel, err := filepath.Rel(e.GetEnvironmentOutput(), path)
if err != nil {
logrus.Errorf("Unable to make path (%s) relative to output (%s) : %s ", path, e.GetEnvironmentOutput(), err)

Fix

  • Make sure all the paths are absolute before giving to these functions. We should be using absolute paths everywhere.
  • Also add a if parent == "" || child == "" { return false; } in the IsParent function
@HarikrishnanBalagopal HarikrishnanBalagopal changed the title bug: doesn't handle the case where paths are empty properly bug: IsParent doesn't handle the case where paths are empty properly Dec 8, 2023
@akagami-harsh
Copy link
Contributor

hey @HarikrishnanBalagopal, i would like to work on this issue. can you please assign this to me

@akagami-harsh
Copy link
Contributor

Make sure all the paths are absolute before giving to these functions. We should be using absolute paths everywhere.

Do I need to make changes throughout the entire repository where the isParent function is being called

@HarikrishnanBalagopal
Copy link
Contributor Author

HarikrishnanBalagopal commented Jan 5, 2024

Make sure all the paths are absolute before giving to these functions. We should be using absolute paths everywhere.

Do I need to make changes throughout the entire repository where the isParent function is being called

Not only the places where IsParent is called, internally everywhere we should be using absolute paths.
You can use a bunch of print statements to check where the relative paths are coming from and fix it at the source.

There are a few places where relative paths may be necessary but I can't recall right now where that is.

@akagami-harsh
Copy link
Contributor

Make sure all the paths are absolute before giving to these functions. We should be using absolute paths everywhere.

Do I need to make changes throughout the entire repository where the isParent function is being called

Not only the places where IsParent is called, internally everywhere we should be using absolute paths. You can use a bunch of print statements to check where the relative paths are coming from and fix it at the source.

There are a few places where relative paths may be necessary but I can't recall right now where that is.

IsParent already uses the filepath.Abs() to convert path form relative to absolute, then why do we need to pass absoulte path.

func IsParent(child, parent string) bool {
	var err error
	child, err = filepath.Abs(child)
	if err != nil {
		logrus.Fatalf("Failed to make the path %s absolute. Error: %s", child, err)
	}
	parent, err = filepath.Abs(parent)
	if err != nil {
		logrus.Fatalf("Failed to make the path %s absolute. Error: %s", parent, err)
	}
	if parent == "/" {
		return true
	}

@HarikrishnanBalagopal
Copy link
Contributor Author

HarikrishnanBalagopal commented Jan 25, 2024

Make sure all the paths are absolute before giving to these functions. We should be using absolute paths everywhere.

Do I need to make changes throughout the entire repository where the isParent function is being called

Not only the places where IsParent is called, internally everywhere we should be using absolute paths. You can use a bunch of print statements to check where the relative paths are coming from and fix it at the source.
There are a few places where relative paths may be necessary but I can't recall right now where that is.

IsParent already uses the filepath.Abs() to convert path form relative to absolute, then why do we need to pass absoulte path.

func IsParent(child, parent string) bool {
	var err error
	child, err = filepath.Abs(child)
	if err != nil {
		logrus.Fatalf("Failed to make the path %s absolute. Error: %s", child, err)
	}
	parent, err = filepath.Abs(parent)
	if err != nil {
		logrus.Fatalf("Failed to make the path %s absolute. Error: %s", parent, err)
	}
	if parent == "/" {
		return true
	}

because that's the bug. The path in the calling context is a relative path (or empty string) and the path inside the isParent is an absolute path (empty string becomes slash in wasm after combining with the PWD /). See the first post for more details.

In general we try to use only absolute paths in all the internal functions to avoid these sort of conversions and associated bugs.

@satyampsoni
Copy link

@akagami-harsh are you working it?
I have made the changes, if you are not working please let me know I'll make the pr

@akagami-harsh
Copy link
Contributor

@akagami-harsh are you working it?
I have made the changes, if you are not working please let me know I'll make the pr

@satyampsoni Go for it.

@akagami-harsh akagami-harsh removed their assignment Mar 17, 2024
@HarikrishnanBalagopal HarikrishnanBalagopal added the kind/bug Categorizes issue or PR as related to a bug. label Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
Status: Backlog
Development

Successfully merging a pull request may close this issue.

3 participants