upstream: Allow for different (but POSIX compliant) behaviour of

basename(3) and prevent a use-after-free in that case in the new sftp-compat
code.

POSIX allows basename(3) to either return a pointer to static storage
or modify the passed string and return a pointer to that.  OpenBSD does
the former and works as is, but on other platforms "filename" points
into "tmp" which was just freed.  This makes the freeing of tmp
consistent with the other variable in the loop.

Pinpointed by the -portable Valgrind regress test.  ok djm@ deraadt@

OpenBSD-Commit-ID: 750f3c19bd4440e4210e30dd5d7367386e833374

diff --git a/scp.c b/scp.c
index 682d062ff0f1e6792f8f5058488230dfb87c8c6b..9cfcfd84000cbfbe96cb94cc33982c45fc38ed03 100644 (file)
--- a/scp.c
+++ b/scp.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: scp.c,v 1.216 2021/08/02 23:38:27 djm Exp $ */
+/* $OpenBSD: scp.c,v 1.217 2021/08/04 01:34:55 dtucker Exp $ */
 /*
  * scp - secure remote copy.  This is basically patched BSD rcp which
  * uses ssh to do the data transfer (instead of using rcmd).
@@ -1461,11 +1461,9 @@ sink_sftp(int argc, char *dst, const char *src, struct sftp_conn *conn)
                tmp = xstrdup(g.gl_pathv[i]);
                if ((filename = basename(tmp)) == NULL) {
                        error("basename %s: %s", tmp, strerror(errno));
-                       free(tmp);
                        err = -1;
                        goto out;
                }
-               free(tmp);
 
                if (local_is_dir(dst))
                        abs_dst = path_append(dst, filename);
@@ -1484,10 +1482,13 @@ sink_sftp(int argc, char *dst, const char *src, struct sftp_conn *conn)
                }
                free(abs_dst);
                abs_dst = NULL;
+               free(tmp);
+               tmp = NULL;
        }
 
 out:
        free(abs_src);
+       free(tmp);
        globfree(&g);
        if (err == -1) {
                fatal("Failed to download file '%s'", src);