Opened 5 years ago

Closed 5 years ago

#20871 closed defect (fixed)

Fix a few more issues with sage-uncompress-spkg

Reported by: embray Owned by:
Priority: minor Milestone: sage-7.3
Component: build Keywords:
Cc: Merged in:
Authors: Erik Bray Reviewers: Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: 0c2c9d4 (Commits, GitHub, GitLab) Commit: 0c2c9d4b06da052bdf9a7e2e749af82fc2f2bea4
Dependencies: Stopgaps:

Status badges

Description

Fixes 2 issues:

  1. An outright bug following from #20721: If the files in a tarball are not all under a top-level directory (shouldn't be the case, but possible), the -d flag did not work as advertised.
  1. Always unset setuid and setgid flags. There's no reason they should be set in a source tarball (I don't think?) and it will mitigate issues like #20870

Change History (21)

comment:1 Changed 5 years ago by embray

  • Status changed from new to needs_review

comment:2 Changed 5 years ago by embray

  • Type changed from PLEASE CHANGE to defect

comment:3 Changed 5 years ago by mkoeppe

  • Reviewers set to Matthias Koeppe
  • Status changed from needs_review to positive_review

comment:4 Changed 5 years ago by mkoeppe

  • Status changed from positive_review to needs_work

There is still an issue with zip files.

sage -i bliss gives:

[bliss-0.73] Found local metadata for bliss-0.73
[bliss-0.73] Using cached file /Users/mkoeppe/cvs/sage/upstream/bliss-0.73.zip
[bliss-0.73] bliss-0.73
[bliss-0.73] ====================================================
[bliss-0.73] Setting up build directory for bliss-0.73
[bliss-0.73] Traceback (most recent call last):
[bliss-0.73]   File "/Users/mkoeppe/cvs/sage/build/bin/sage-uncompress-spkg", line 260, in <module>
[bliss-0.73]     sys.exit(main())
[bliss-0.73]   File "/Users/mkoeppe/cvs/sage/build/bin/sage-uncompress-spkg", line 212, in main
[bliss-0.73]     archive = cls.open(filename)
[bliss-0.73] TypeError: unbound method open() must be called with SageZipFile instance as first argument (got str instance instead)
[bliss-0.73] Error: failed to extract /Users/mkoeppe/cvs/sage/upstream/bliss-0.73.zip

comment:5 follow-up: Changed 5 years ago by embray

There is still an issue with zip files.

Was this reported before?

Might as well fix it as part of this ticket too while I'm fixing bugs.

It looks like the ZipFile class is inconsistent with the TarFile class, where open is a classmethod. I think I knew that but forgot this time.

comment:6 in reply to: ↑ 5 Changed 5 years ago by mkoeppe

Replying to embray:

There is still an issue with zip files.

Was this reported before?

I didn't see a report; but zip files did work in the past. Perhaps introduced in #20721? I didn't catch it at this time.

Might as well fix it as part of this ticket too while I'm fixing bugs.

Thanks.

comment:7 Changed 5 years ago by vbraun

  • Branch changed from u/embray/unpack-tarball-src to 30ac0a68a1e8cc0bbfd2b7e48c7aee119dca80f2
  • Resolution set to fixed
  • Status changed from needs_work to closed

comment:8 Changed 5 years ago by vbraun

  • Commit 30ac0a68a1e8cc0bbfd2b7e48c7aee119dca80f2 deleted
  • Resolution fixed deleted
  • Status changed from closed to new

comment:9 Changed 5 years ago by vbraun

  • Branch changed from 30ac0a68a1e8cc0bbfd2b7e48c7aee119dca80f2 to u/embray/unpack-tarball-src
  • Commit set to 30ac0a68a1e8cc0bbfd2b7e48c7aee119dca80f2

New commits:

3ea4482Fix bug with -d option when there is no single top-level directory in the tarball--the script was not doing what it's supposed to in this case
30ac0a6Also unset SUID and SGID flags on extracted files, to prevent issues like https://trac.sagemath.org/ticket/20870#

comment:10 Changed 5 years ago by embray

What just happened here?

comment:11 Changed 5 years ago by git

  • Commit changed from 30ac0a68a1e8cc0bbfd2b7e48c7aee119dca80f2 to 40449bf2bde7a7a7ca214fb8c84bdfb08e97cf6d

Branch pushed to git repo; I updated commit sha1. New commits:

40449bfFix zip file opening.

comment:12 Changed 5 years ago by vbraun

I merged the positively-reviewed ticket, all tests passed, and then I closed the ticket.

comment:13 Changed 5 years ago by embray

The status was needs_work though. I just pushed another commit that should have been included.

comment:14 Changed 5 years ago by vbraun

The status was positive review when I merged the ticket.

comment:15 Changed 5 years ago by embray

  • Status changed from new to needs_review

comment:16 Changed 5 years ago by mkoeppe

  • Status changed from needs_review to positive_review

Branch works now, but may need merge/rebase.

comment:17 Changed 5 years ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:18 Changed 5 years ago by git

  • Commit changed from 40449bf2bde7a7a7ca214fb8c84bdfb08e97cf6d to 0c2c9d4b06da052bdf9a7e2e749af82fc2f2bea4

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

c6ce4aaFix bug with -d option when there is no single top-level directory in the tarball--the script was not doing what it's supposed to in this case
66d67dcAlso unset SUID and SGID flags on extracted files, to prevent issues like https://trac.sagemath.org/ticket/20870#
0c2c9d4Fix zip file opening.

comment:19 Changed 5 years ago by embray

  • Status changed from needs_work to needs_review

Rebased.

comment:20 Changed 5 years ago by mkoeppe

  • Status changed from needs_review to positive_review

comment:21 Changed 5 years ago by vbraun

  • Branch changed from u/embray/unpack-tarball-src to 0c2c9d4b06da052bdf9a7e2e749af82fc2f2bea4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.