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.

Sign up for GitHub

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

Jump to bottom

archive/zip: AddFS with a symlink results in an empty file #61875

Closed
mauri870 opened this issue Aug 8, 2023 · 4 comments
Closed

archive/zip: AddFS with a symlink results in an empty file #61875

mauri870 opened this issue Aug 8, 2023 · 4 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Milestone
Go1.22

Comments

@mauri870
Copy link
Member

mauri870 commented Aug 8, 2023 •
edited

What version of Go are you using (go version)?

$ go version
tip

Does this issue reproduce with the latest release?

Yes, this is a tip (1.22) only issue.

What did you do?

Tried to call AddFS with a fs.FS with symlinks.

package main

import (
	"archive/zip"
	"log"
	"os"
)

func main() {
	fsys := os.DirFS("./dir") // ./dir contains symlinks inside
	f, err := os.OpenFile("file.zip", os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0755)
	if err != nil {
		log.Fatal(err)
	}
	defer f.Close()

	zw := zip.NewWriter(f)
	defer zw.Close()
	zw.AddFS(fsys)
}

What did you expect to see?

Either an error or the proper file contents.

What did you see instead?

Files that are symlinks result in an empty file inside the zip archive.

Had a discussion on this CL about symlink support in zip files.
We have the proposal for fs.ReadLinkFS that could somewhat make this possible to implement, but the proposal is currently on hold.

I see a couple options to be weighted:

  • Keep it as it is, file gets added to the zip but is empty
  • Detect files that are symlinks and then return an error upon calling AddFS
  • Don't return an error and don't add the file to the zip. This seems to be the behavior with the zip command line tool. There is a logged message zip warning: name not matched: ./dir/iamasymlink but the zip gets created, skipping the symlinks.
@ianlancetaylor ianlancetaylor added this to the Go1.22 milestone Aug 8, 2023
@ianlancetaylor ianlancetaylor added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker labels Aug 8, 2023
@ianlancetaylor
Copy link
Contributor

CC @dsnet

@dsnet
Copy link
Member

dsnet commented Aug 8, 2023 •
edited

Of the options, encoding an empty file seems to be the worst.

I'd argue for returning an error for the time being. It is generally more acceptable to convert an error into a properly handled case in the future, than the other way around. I'm not opposed to just ignoring non-regular files as well.

There is an seldom used extension to zip that can encode Unix-specific files like symlinks, devices, etc., but it's not clear to me that we should implement that.

I can imagine a community of helper fs.FS functions that would make handling this easier. For example, you could imagine a function that wraps fs.FS and returns one that only reports regular files. Such a helper seems broadly useful.

@mauri870
Copy link
Member Author

mauri870 commented Aug 8, 2023 •
edited

Proposal for fs.ReadLinkFS seems promising. We could try to type assert it and then read from the source of the symlink, then fs.FS should work as expected.

As you said from the choices we have returning an error is the most correct way. In the future if we support this the transition will be seamless.

Iwill push a fix for the AddFS in archive/zip as well as fixing my open CL in archive/tar

mauri870 added a commit to mauri870/go that referenced this issue Aug 9, 2023
@mauri870
archive/zip: AddFS can't process filesystem with symlinks
df19759 
When a filesystem with symlinks is used to invoke AddFS the resulting
file inside the zip archive is empty if the file is a symbolic link.

In this case we can be explicit and return an error.

Fixes golang#61875
@mauri870 mauri870 mentioned this issue Aug 9, 2023
Closed
mauri870 added a commit to mauri870/go that referenced this issue Aug 9, 2023
@mauri870
archive/zip: AddFS can't process filesystem with symlinks
a6e35b2 
When a filesystem with symlinks is used to invoke AddFS the resulting
file inside the zip archive is empty if the file is a symbolic link.

In this case we can be explicit and return an error.

Fixes golang#61875
@gopherbot
Copy link

Change https://go.dev/cl/517475 mentions this issue: archive/zip: AddFS can't process filesystem with symlinks

mauri870 added a commit to mauri870/go that referenced this issue Aug 9, 2023
@mauri870
archive/zip: reject non-regular files in AddFS
22cdae8 
When a filesystem with non-regular files is used
the resulting files inside the zip archive are empty.

In this case we can be explicit and return an error.

Fixes golang#61875
mauri870 added a commit to mauri870/go that referenced this issue Aug 9, 2023
@mauri870
archive/zip: reject non-regular files in AddFS
c614f69 
When a filesystem with non-regular files is used
the resulting files inside the zip archive are empty.

In this case we can be explicit and return an error.

Fixes golang#61875
mauri870 added a commit to mauri870/go that referenced this issue Sep 7, 2023
@mauri870
archive/zip: reject non-regular files in AddFS
5e53dcf 
When a filesystem with non-regular files is used
the resulting files inside the zip archive are empty.

In this case we can be explicit and return an error.

Fixes golang#61875
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Projects
None yet
Milestone
Go1.22
Development

Successfully merging a pull request may close this issue.

archive/zip: reject non-regular files in AddFS mauri870/go
4 participants
@ianlancetaylor @dsnet @gopherbot @mauri870