sftp method no longer uses temporary file name during upload

Bug #1762572 reported by David Lechner
12
This bug affects 2 people
Affects Status Importance Assigned to Milestone
dput (Ubuntu)
Fix Released
Low
Unassigned

Bug Description

In previous versions of dput, the sftp method would use a temporary file name while uploading. For example:

linux-headers-4.4.61-20-ev3dev-rpi_1~1_armhf.deb.tmp.1492202382.950381994.19485.970636164

In bionic with package version 1.0.1ubuntu1, it no longer does this.

I have a remote script that it triggered by a file system watcher that looks for the creation of a new .changes file. Since the file is created with this line:

with sftp_client.file(remote_path, "w") as remote_fileobj:

the initial file is empty and as a result, my script sent me this message from reprepro:

Unexpected empty file 'raspberrypi-firmware_1.20180409-1ev3dev1_armhf.changes'!
There have been errors!

This script was working with previous versions of dput.

Tags: patch
Revision history for this message
David Lechner (dlech) wrote :

It looks like this can be fixed by copying (parts of) the _put() method from bzrlib

    def _put(self, abspath, f, mode=None):
        """Helper function so both put() and copy_abspaths can reuse the code"""
        tmp_abspath = '%s.tmp.%.9f.%d.%d' % (abspath, time.time(),
                        os.getpid(), random.randint(0,0x7FFFFFFF))
        fout = self._sftp_open_exclusive(tmp_abspath, mode=mode)
        closed = False
        try:
            try:
                fout.set_pipelined(True)
                length = self._pump(f, fout)
            except (IOError, paramiko.SSHException), e:
                self._translate_io_exception(e, tmp_abspath)
            # XXX: This doesn't truly help like we would like it to.
            # The problem is that openssh strips sticky bits. So while we
            # can properly set group write permission, we lose the group
            # sticky bit. So it is probably best to stop chmodding, and
            # just tell users that they need to set the umask correctly.
            # The attr.st_mode = mode, in _sftp_open_exclusive
            # will handle when the user wants the final mode to be more
            # restrictive. And then we avoid a round trip. Unless
            # paramiko decides to expose an async chmod()

            # This is designed to chmod() right before we close.
            # Because we set_pipelined() earlier, theoretically we might
            # avoid the round trip for fout.close()
            if mode is not None:
                self._get_sftp().chmod(tmp_abspath, mode)
            fout.close()
            closed = True
            self._rename_and_overwrite(tmp_abspath, abspath)
            return length
        except Exception, e:
            # If we fail, try to clean up the temporary file
            # before we throw the exception
            # but don't let another exception mess things up
            # Write out the traceback, because otherwise
            # the catch and throw destroys it
            import traceback
            mutter(traceback.format_exc())
            try:
                if not closed:
                    fout.close()
                self._get_sftp().remove(tmp_abspath)
            except:
                # raise the saved except
                raise e
            # raise the original with its traceback if we can.
            raise

Revision history for this message
Launchpad Janitor (janitor) wrote :

Status changed to 'Confirmed' because the bug affects multiple users.

Changed in dput (Ubuntu):
status: New → Confirmed
Revision history for this message
David Lechner (dlech) wrote :

Here is a patch that I have been using for a while.

Revision history for this message
Sebastien Bacher (seb128) wrote :

Thank you for your bug report, is the use of the temporary filename really a feature or is that an implementation detail you happened to rely on?

Revision history for this message
David Lechner (dlech) wrote :

It is a feature to me. I can't think of an easy way to work around it otherwise.

Using a temporary file like this seems common. In fact dput used to do this, which is why I consider it a regression.

Similar advice here: https://stackoverflow.com/questions/25119076/how-to-do-an-atomic-sftp-file-transfer-using-jsch-so-that-the-file-is-not-acces

Revision history for this message
Ubuntu Foundations Team Bug Bot (crichton) wrote :

The attachment "0001-fix-atomic-upload-regression-for-sftp-method.patch" seems to be a patch. If it isn't, please remove the "patch" flag from the attachment, remove the "patch" tag, and if you are a member of the ~ubuntu-reviewers, unsubscribe the team.

[This is an automated message performed by a Launchpad user owned by ~brian-murray, for any issues please contact him.]

tags: added: patch
Revision history for this message
Julian Andres Klode (juliank) wrote :

Thanks for the patch! I think Python has built-in functions for generating files names we could use, tough, that might look nicer.

I'll have a look later. I also need to update my git Branch for upstreaming.

Changed in dput (Ubuntu):
status: Confirmed → Triaged
Mathew Hodson (mhodson)
Changed in dput (Ubuntu):
importance: Undecided → Low
Mathew Hodson (mhodson)
summary: - regression: sftp method no longer uses temporary file name during upload
+ sftp method no longer uses temporary file name during upload
Revision history for this message
Julian Andres Klode (juliank) wrote :

Merged in 1.0.3ubuntu1.

Changed in dput (Ubuntu):
status: Triaged → Fix Committed
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package dput - 1.0.3ubuntu1

---------------
dput (1.0.3ubuntu1) eoan; urgency=medium

  [ Julian Andres Klode ]
  * Merge from Debian unstable. Remaining changes:
    - dput:
      + Recognize 0ubuntu1 as a debian version that requires
        orig.tar.gz to be included in the upload.
      + Correctly handle cases where a non-existant host or no host at all is
        supplied when -o option is passed. (Thanks to David Futcher for
        spotting)
      + Be more careful about setting a variable in a section that does not
        exist in host argument handling.
    - dput.1: Updated to document host argument feature and sftp support.
    - dput.cf:
      + Set 'default_main_host = ubuntu'
      + Set 'progress_indicator = 2'
      + Updated ppa stanza to make use of argument support.
    - dput.cf.5: Updated to note support for sftp transport and host args.
    - dput.cf: Switch ubuntu stanza to upload to "/ubuntu" rather than "/"
      (LP: #1340130).
    - dput.cf: Drop trailing "/ubuntu" from ppa stanza, to support the new
      form of the upload path needed for PPAs based on derived distributions
      (LP: #1340130).
    - Implement a new sftp method that connects via openssh and then
      uses paramiko's sftp support. Some code copied from bzr.
  * sftp: Use bigger chunks when copying files (LP: #1814791)
  * sftp: Handle exceptions during upload, try to remove file

  [ David Lechner ]
  * sftp: fix atomic upload regression for sftp method (LP: #1762572)

dput (1.0.3) unstable; urgency=medium

  * The “سعد راشد محمد الفقيه‎‎ (Sa'ad Rashed Mohammad al-Faqih)” release.

  [ Ben Finney ]
  * Specify current VCS for this code base.
  * Declare “Standards-Version: 4.3.0”. No additional changes needed.
  * Declare Debhelper compatibility level 12.
  * debian/compat:
    * Remove obsolescent configuration file.
  * Update publication years in copyright notices.

  [ Ondřej Nový ]
  * Remove specification of minimum Python versions.
    The required versions are now in all supported Debian releases.

 -- Julian Andres Klode <email address hidden> Mon, 22 Apr 2019 12:31:01 +0200

Changed in dput (Ubuntu):
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.