sftp method no longer uses temporary file name during upload

Bug #1762572 reported by David Lechner on 2018-04-09
16
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
Changed in dput (Ubuntu):
importance: Undecided → Low
summary: - regression: sftp method no longer uses temporary file name during upload
+ sftp method no longer uses temporary file name during upload
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers