Re: ftp(1) double free

2020-07-04 Thread Theo Buehler
On Sun, Jun 21, 2020 at 02:23:54AM +0200, Jeremie Courreges-Anglas wrote:
> 
> Small bug, but still...
> 
> naddy@ reported a double free when interrupting (^C) ftp(1) at the end
> or url_get().  If that happens, the SIGINT handler longjmps and the
> cleanup path is taken a second time.  To avoid this, restore the
> previous SIGINT handler in each possible error path.  I chose to restore
> it earlier in the successful path to avoid too much duplication.
> file_get() gets the same treatment.
> 
> Note that naddy hit this because of another bug causing ftp_close()
> to stall, and this depends on the server you're talking to.  You can
> insert a sleep(10); call before "return (rval);" to help reproduce the
> crash.
> 
> ok?
> 

ok tb

> 
> Index: fetch.c
> ===
> RCS file: /d/cvs/src/usr.bin/ftp/fetch.c,v
> retrieving revision 1.195
> diff -u -p -p -u -r1.195 fetch.c
> --- fetch.c   20 Jun 2020 09:59:48 -  1.195
> +++ fetch.c   21 Jun 2020 00:12:10 -
> @@ -261,6 +261,7 @@ file_get(const char *path, const char *o
>   for (cp = buf; len > 0; len -= wlen, cp += wlen) {
>   if ((wlen = write(out, cp, len)) == -1) {
>   warn("Writing %s", savefile);
> + signal(SIGINT, oldintr);
>   signal(SIGINFO, oldinti);
>   goto cleanup_copy;
>   }
> @@ -274,6 +275,7 @@ file_get(const char *path, const char *o
>   }
>   }
>   save_errno = errno;
> + signal(SIGINT, oldintr);
>   signal(SIGINFO, oldinti);
>   if (hash && !progress && bytes > 0) {
>   if (bytes < mark)
> @@ -288,7 +290,6 @@ file_get(const char *path, const char *o
>   progressmeter(1, NULL);
>   if (verbose)
>   ptransfer(0);
> - (void)signal(SIGINT, oldintr);
>  
>   rval = 0;
>  
> @@ -1032,6 +1033,7 @@ noslash:
>   oldinti = signal(SIGINFO, psummary);
>   if (chunked) {
>   error = save_chunked(fin, tls, out, buf, buflen);
> + signal(SIGINT, oldintr);
>   signal(SIGINFO, oldinti);
>   if (error == -1)
>   goto cleanup_url_get;
> @@ -1041,6 +1043,7 @@ noslash:
>   for (cp = buf; len > 0; len -= wlen, cp += wlen) {
>   if ((wlen = write(out, cp, len)) == -1) {
>   warn("Writing %s", savefile);
> + signal(SIGINT, oldintr);
>   signal(SIGINFO, oldinti);
>   goto cleanup_url_get;
>   }
> @@ -1054,6 +1057,7 @@ noslash:
>   }
>   }
>   save_errno = errno;
> + signal(SIGINT, oldintr);
>   signal(SIGINFO, oldinti);
>   if (hash && !progress && bytes > 0) {
>   if (bytes < mark)
> @@ -1080,7 +1084,6 @@ noslash:
>  
>   if (verbose)
>   ptransfer(0);
> - (void)signal(SIGINT, oldintr);
>  
>   rval = 0;
>   goto cleanup_url_get;
> 
> 
> -- 
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE
> 



ftp(1) double free

2020-06-20 Thread Jeremie Courreges-Anglas


Small bug, but still...

naddy@ reported a double free when interrupting (^C) ftp(1) at the end
or url_get().  If that happens, the SIGINT handler longjmps and the
cleanup path is taken a second time.  To avoid this, restore the
previous SIGINT handler in each possible error path.  I chose to restore
it earlier in the successful path to avoid too much duplication.
file_get() gets the same treatment.

Note that naddy hit this because of another bug causing ftp_close()
to stall, and this depends on the server you're talking to.  You can
insert a sleep(10); call before "return (rval);" to help reproduce the
crash.

ok?


Index: fetch.c
===
RCS file: /d/cvs/src/usr.bin/ftp/fetch.c,v
retrieving revision 1.195
diff -u -p -p -u -r1.195 fetch.c
--- fetch.c 20 Jun 2020 09:59:48 -  1.195
+++ fetch.c 21 Jun 2020 00:12:10 -
@@ -261,6 +261,7 @@ file_get(const char *path, const char *o
for (cp = buf; len > 0; len -= wlen, cp += wlen) {
if ((wlen = write(out, cp, len)) == -1) {
warn("Writing %s", savefile);
+   signal(SIGINT, oldintr);
signal(SIGINFO, oldinti);
goto cleanup_copy;
}
@@ -274,6 +275,7 @@ file_get(const char *path, const char *o
}
}
save_errno = errno;
+   signal(SIGINT, oldintr);
signal(SIGINFO, oldinti);
if (hash && !progress && bytes > 0) {
if (bytes < mark)
@@ -288,7 +290,6 @@ file_get(const char *path, const char *o
progressmeter(1, NULL);
if (verbose)
ptransfer(0);
-   (void)signal(SIGINT, oldintr);
 
rval = 0;
 
@@ -1032,6 +1033,7 @@ noslash:
oldinti = signal(SIGINFO, psummary);
if (chunked) {
error = save_chunked(fin, tls, out, buf, buflen);
+   signal(SIGINT, oldintr);
signal(SIGINFO, oldinti);
if (error == -1)
goto cleanup_url_get;
@@ -1041,6 +1043,7 @@ noslash:
for (cp = buf; len > 0; len -= wlen, cp += wlen) {
if ((wlen = write(out, cp, len)) == -1) {
warn("Writing %s", savefile);
+   signal(SIGINT, oldintr);
signal(SIGINFO, oldinti);
goto cleanup_url_get;
}
@@ -1054,6 +1057,7 @@ noslash:
}
}
save_errno = errno;
+   signal(SIGINT, oldintr);
signal(SIGINFO, oldinti);
if (hash && !progress && bytes > 0) {
if (bytes < mark)
@@ -1080,7 +1084,6 @@ noslash:
 
if (verbose)
ptransfer(0);
-   (void)signal(SIGINT, oldintr);
 
rval = 0;
goto cleanup_url_get;


-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE