Skip to content
This repository has been archived by the owner on Feb 24, 2020. It is now read-only.

tar: prefix hard link destination with extraction target #1827

Closed
wants to merge 1 commit into from

Conversation

cgonyeo
Copy link
Member

@cgonyeo cgonyeo commented Dec 2, 2015

If the extraction is not running in a chroot, the file to hard link
against must be prefaced by the target for the extraction, otherwise the
file that's being linked to will not exist and the link will fail.

Fixes containers/build#139.

@jonboulle
Copy link
Contributor

Egads. Great catch. LGTM. Can we test, though?

@jonboulle
Copy link
Contributor

Also s/extration/extraction/

@cgonyeo cgonyeo changed the title tar: prefix hard link destination with extration target tar: prefix hard link destination with extraction target Dec 7, 2015
@cgonyeo
Copy link
Member Author

cgonyeo commented Dec 7, 2015

Test added, and as a bonus all tests now run against both ExtractTar and ExtractTarInsecure

func TestExtractTarHardLink(t *testing.T) {
for _, insecure := range []bool{false, true} {
if !insecure && !sys.HasChrootCapability() {
t.Skipf("chroot capability not available. Disabling test.")
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't quite right because you're skipping both test cases (insecure && !insecure)

Copy link
Member Author

Choose a reason for hiding this comment

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

whoops

@cgonyeo
Copy link
Member Author

cgonyeo commented Dec 7, 2015

No longer unintentionally skipping parts of tests.

func extractTarInsecureHelper(t *testing.T, tr *tar.Reader, target string) error {
editor, err := NewUidShiftingFilePermEditor(uid.NewBlankUidRange())
if err != nil {
t.Errorf("unexpected error: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

return err

@cgonyeo cgonyeo force-pushed the tar-fix-hardlinks branch 2 times, most recently from 8976829 to a2a3117 Compare December 7, 2015 23:06
}
for _, insecure := range []bool{false, true} {
if !insecure && !sys.HasChrootCapability() {
t.Logf("chroot capability not available. Skipping part of test.")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think splitting these out is preferable, they're quite distinct paths conceptually:

func TestExtractTarFoldersChroot(t *testing.T) {
  if !sys.HasChrootCapability() {
    t.Skipf(...)
  }
  testExtractTarFolders(t, true) 
}

func TestExtractTarFoldersInsecure(t *testing.T) {
  testExtractTarFolders(t, false)
}

func testExtractTarFolders(t *testing.T, chroot bool) 

(The bool is kinda icky, ideally testExtractTarFolders could have a nicer signature with some kind of func() but I'm not sure what it looks like off the top of my head)

@cgonyeo cgonyeo closed this Dec 8, 2015
@cgonyeo cgonyeo reopened this Dec 8, 2015
@cgonyeo
Copy link
Member Author

cgonyeo commented Dec 8, 2015

Someone should kick semaphore for me :/

If the extraction is not running in a chroot, the file to hard link
against must be prefaced by the target for the extraction, otherwise the
file that's being linked to will not exist and the link will fail.
@cgonyeo
Copy link
Member Author

cgonyeo commented Dec 8, 2015

Tests split.

@krnowak
Copy link
Collaborator

krnowak commented Dec 8, 2015

Semaphore decided to remove the branch from testing as seen on https://semaphoreci.com/coreos/rkt/

I guess you'll need to create a new PR. :/

@alban
Copy link
Member

alban commented Dec 8, 2015

It needs to be rebased too.

@alban
Copy link
Member

alban commented Dec 8, 2015

After closing/reopening the PR, we cannot kick Semaphore anymore... For next time, Semaphore can be kicked by pushing the branch again ;)

$ git checkout pr-branch
$ git commit --amend --no-edit
$ git push --force my-repo pr-branch

@alban
Copy link
Member

alban commented Dec 8, 2015

LGTM if it is rebased and if Semaphore is green.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants