sftp method no longer uses temporary file name during upload

Bug #1762572 reported by David Lechner on 2018-04-09
12
This bug affects 2 people
Affects Status Importance Assigned to Milestone
dput (Ubuntu)
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.

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

Launchpad Janitor (janitor) wrote :

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

Changed in dput (Ubuntu):
status: New → Confirmed
David Lechner (dlech) wrote :

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

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?

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

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
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) on 2019-02-09
Changed in dput (Ubuntu):
importance: Undecided → Low
Mathew Hodson (mhodson) on 2019-03-10
summary: - regression: sftp method no longer uses temporary file name during upload
+ sftp method no longer uses temporary file name during upload
Julian Andres Klode (juliank) wrote :

Merged in 1.0.3ubuntu1.

Changed in dput (Ubuntu):
status: Triaged → Fix Committed
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  Edit
Everyone can see this information.

Other bug subscribers