[Bug 2948] implement "copy-data" sftp extension

[bugzilla-daemon at bugzilla.mindrot.org]

https://bugzilla.mindrot.org/show_bug.cgi?id=2948

--- Comment #8 from Mike Frysinger <vapier at gentoo.org> ---
(In reply to Darren Tucker from comment #3)

thx for the reviews.

> > [...] This avoids needing to
> >+download the data before uploading it across the network twice.
> 
> Why uploading twice?

i was referring to network transfers rather than upload specifically. 
i'll rephrase to be more clear.

> >+	{ "copy-data", "copy-data", 0, process_extended_copy_data, 1 },
> 
> This is a protocol violation.  The server is implementing
> secsh-filexfer-02 which does not specify copy-data.  Vendor
> extensions such as this need an @[domainname] suffix.

the secsh-filexfer-02 spec [1] says:
> The name should be of the form "name at domain", where the domain is the DNS domain name of the organization defining the extension.
> Additional names that are not of this format may be defined later by the IETF.

since the IETF has an RFC that defines "copy-data" [2], and that's what
i implemented, that's why i think it's appropriate to omit the @ vendor
suffix.

[1] https://tools.ietf.org/html/draft-ietf-secsh-filexfer-02#section-4
[2]
https://tools.ietf.org/html/draft-ietf-secsh-filexfer-extensions-00#section-7

> >+				ret = read(read_fd, buf, len);
> 
> ret should be ssize_t not int.

i'm aware of the API nuance, but i went with "follow existing style". 
specifically, process_read() uses int with read(), and i mirrored that.
 if that func is an oversight and should also be ssize_t, i'm happy to
change.

(In reply to Darren Tucker from comment #6)
> >+.It Ic copy Ar oldpath Ar newpath
> 
> the man page says "copy" but the code says "cp".  Personally I
> prefer "cp".

nice catch.  sftp uses aliases for other commands, so i'll keep "copy"
as the documented one, and keep "cp" as a shorter alias.  this keeps
with existing "chdir" and "cd" behavior.

> > +	} else {
> >+		mode = 0666;
> 
> I'm not sure we should be making world writable files by default.

it isn't by default exactly ... that mode is used if the server doesn't
respond with mode info for the source file which should be fairly
unusual.

ignoring that, the server umask should apply.  so if the user's umask
is like 0022, i'd expect this to be 0644.

that said, this is also a case of me copying existing style -- this is
how do_download() behaves.

i can switch it to 0644 if people really prefer, but it seemed like all
things considered, 0666 is correct.

> >+	if (old_handle == NULL) {
> >+		sshbuf_free(msg);
> >+		return -1;
> >+	}
> >+
> >+	/* Open the new file for writing */
> [...]
> >+	if (new_handle == NULL) {
> >+		sshbuf_free(msg);
> >+		return -1;
> +	}
> 
> I think this leaks old_handle if opening new_handle fails.  It'd
> probably be cleaner to initialise status and the handles to -1/NULL
> then do a "goto out" and have all the cleanup in one place.

i'll add an explicit free.  i didn't use goto style since this file
doesn't do that for error handling in general.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.