Mailing List Archive

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

--- Comment #9 from Damien Miller <djm@mindrot.org> ---
Comment on attachment 3344
--> https://bugzilla.mindrot.org/attachment.cgi?id=3344
sftp server copy-data extension

looks good - some minor comments

>diff --git a/PROTOCOL b/PROTOCOL
>index f75c1c0ae5b0..04a392db33be 100644
...
>+static void
>+process_extended_copy_data(u_int32_t id)
...
>+ /* Disallow reading & writing to the same handle */
>+ if (read_handle == write_handle || read_fd < 0 || write_fd < 0) {

I think this should also check that both the read and write handles do
not refer to the same path? (use handle_to_name())

>+ status = SSH2_FX_FAILURE;
>+ } else {

nit: prefer "goto out" over nesting if/else

>+ if (lseek(read_fd, read_off, SEEK_SET) < 0) {
>+ status = errno_to_portable(errno);
>+ error("process_extended_copy_data: read_seek failed");

nit: ditto

>+ } else if (!(handle_to_flags(write_handle) & O_APPEND) &&
>+ lseek(write_fd, write_off, SEEK_SET) < 0) {
>+ status = errno_to_portable(errno);
>+ error("process_extended_copy_data: write_seek failed");

nit: prefer __func__ to manual inclusion of function name

>+ } else {
>+ /* Process the request in chunks. */
>+ while (read_len || copy_until_eof) {

nit: prefer explicit comparison against zero (i.e "read_len > 0")

>+
>+ ret = read(read_fd, buf, len);
...
>+ ret = write(write_fd, buf, len);

I think this should use atomicio here to be signal safe.

>+ if ((size_t)ret != len) {
>+ debug2("nothing at all written");
>+ status = SSH2_FX_FAILURE;
>+ break;
>+ }

this block can go away with atomicio

--
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.
_______________________________________________
openssh-bugs mailing list
openssh-bugs@mindrot.org
https://lists.mindrot.org/mailman/listinfo/openssh-bugs