On Tue, Feb 06, 2018 at 03:02:14PM +1300, [email protected] wrote:
>
>
> On Tue, 6 Feb 2018, Theo Buehler wrote:
>
> > In cleanup_url_get, fin and s will be closed a second time, so mark them
> > as invalid after closing them the first time.
> >
> > Another option might be to remove the fclose/close calls, but since this
> > happens right before the recursive call, I'm not sure whether this might
> > run the risk of hitting limits.
>
> I got curious about the 'else if': fin = fdopen(s) is possible,
> and closing both 'fin' and 's' will clobber errno.
>
> How about the following further tweak to eliminate it?
>
> I've also attached a follow-up patch that renames 's' -> 'fd'; searching
> for 's' is infeasible without the compiler's help.
>
I'm ok with both diffs.
> Index: ftp/fetch.c
> ===================================================================
> --- ftp.orig/fetch.c
> +++ ftp/fetch.c
> @@ -636,9 +636,11 @@ noslash:
> }
> } else {
> fin = fdopen(s, "r+");
> + s = -1;
> }
> #else /* !NOSSL */
> fin = fdopen(s, "r+");
> + s = -1;
> #endif /* !NOSSL */
>
> #ifdef SMALL
> @@ -912,10 +914,14 @@ noslash:
> *loctail = '\0';
> if (verbose)
> fprintf(ttyout, "Redirected to %s\n", redirurl);
> - if (fin != NULL)
> + if (fin != NULL) {
> fclose(fin);
> - else if (s != -1)
> + fin = NULL;
> + }
> + if (s != -1) {
> close(s);
> + s = -1;
> + }
> rval = url_get(redirurl, proxyenv, savefile, lastfile);
> free(redirurl);
> goto cleanup_url_get;
> @@ -1039,10 +1045,14 @@ cleanup_url_get:
> free(full_host);
> free(sslhost);
> #endif /* !NOSSL */
> - if (fin != NULL)
> + if (fin != NULL) {
> fclose(fin);
> - else if (s != -1)
> + fin = NULL;
> + }
> + if (s != -1) {
> close(s);
> + s = -1;
> + }
> if (out >= 0 && out != fileno(stdout))
> close(out);
> free(buf);
>
>
> Index: ftp/fetch.c
> ===================================================================
> --- ftp.orig/fetch.c
> +++ ftp/fetch.c
> @@ -189,7 +189,7 @@ url_get(const char *origline, const char
> const char * volatile savefile;
> char * volatile proxyurl = NULL;
> char *credentials = NULL;
> - volatile int s = -1, out = -1;
> + volatile int fd = -1, out = -1;
> volatile sig_t oldintr, oldinti;
> FILE *fin = NULL;
> off_t hashbytes;
> @@ -364,13 +364,13 @@ noslash:
> if (isfileurl) {
> struct stat st;
>
> - s = open(path, O_RDONLY);
> - if (s == -1) {
> + fd = open(path, O_RDONLY);
> + if (fd == -1) {
> warn("Can't open file %s", path);
> goto cleanup_url_get;
> }
>
> - if (fstat(s, &st) == -1)
> + if (fstat(fd, &st) == -1)
> filesize = -1;
> else
> filesize = st.st_size;
> @@ -399,7 +399,7 @@ noslash:
> warn("Can't fstat %s", savefile);
> goto cleanup_url_get;
> }
> - if (lseek(s, st.st_size, SEEK_SET) == -1) {
> + if (lseek(fd, st.st_size, SEEK_SET) == -1) {
> warn("Can't lseek %s", path);
> goto cleanup_url_get;
> }
> @@ -429,7 +429,7 @@ noslash:
> /* Finally, suck down the file. */
> i = 0;
> oldinti = signal(SIGINFO, psummary);
> - while ((len = read(s, buf, buflen)) > 0) {
> + while ((len = read(fd, buf, buflen)) > 0) {
> bytes += len;
> for (cp = buf; len > 0; len -= i, cp += i) {
> if ((i = write(out, cp, len)) == -1) {
> @@ -535,7 +535,7 @@ noslash:
> if (verbose)
> setvbuf(ttyout, NULL, _IOLBF, 0);
>
> - s = -1;
> + fd = -1;
> for (res = res0; res; res = res->ai_next) {
> if (getnameinfo(res->ai_addr, res->ai_addrlen, hbuf,
> sizeof(hbuf), NULL, 0, NI_NUMERICHOST) != 0)
> @@ -543,8 +543,8 @@ noslash:
> if (verbose)
> fprintf(ttyout, "Trying %s...\n", hbuf);
>
> - s = socket(res->ai_family, res->ai_socktype, res->ai_protocol);
> - if (s == -1) {
> + fd = socket(res->ai_family, res->ai_socktype, res->ai_protocol);
> + if (fd == -1) {
> cause = "socket";
> continue;
> }
> @@ -552,17 +552,17 @@ noslash:
> #ifndef SMALL
> if (srcaddr) {
> if (ares->ai_family != res->ai_family) {
> - close(s);
> - s = -1;
> + close(fd);
> + fd = -1;
> errno = EINVAL;
> cause = "bind";
> continue;
> }
> - if (bind(s, ares->ai_addr, ares->ai_addrlen) < 0) {
> + if (bind(fd, ares->ai_addr, ares->ai_addrlen) < 0) {
> save_errno = errno;
> - close(s);
> + close(fd);
> errno = save_errno;
> - s = -1;
> + fd = -1;
> cause = "bind";
> continue;
> }
> @@ -574,14 +574,14 @@ noslash:
> alarmtimer(connect_timeout);
> }
>
> - for (error = connect(s, res->ai_addr, res->ai_addrlen);
> - error != 0 && errno == EINTR; error = connect_wait(s))
> + for (error = connect(fd, res->ai_addr, res->ai_addrlen);
> + error != 0 && errno == EINTR; error = connect_wait(fd))
> continue;
> if (error != 0) {
> save_errno = errno;
> - close(s);
> + close(fd);
> errno = save_errno;
> - s = -1;
> + fd = -1;
> cause = "connect";
> continue;
> }
> @@ -595,7 +595,7 @@ noslash:
>
> #ifndef NOSSL
> if (proxyenv && sslhost)
> - proxy_connect(s, sslhost, credentials);
> + proxy_connect(fd, sslhost, credentials);
> #endif /* !NOSSL */
> break;
> }
> @@ -604,7 +604,7 @@ noslash:
> if (srcaddr)
> freeaddrinfo(ares);
> #endif /* !SMALL */
> - if (s < 0) {
> + if (fd < 0) {
> warn("%s", cause);
> goto cleanup_url_get;
> }
> @@ -630,17 +630,17 @@ noslash:
> tls_error(tls));
> goto cleanup_url_get;
> }
> - if (tls_connect_socket(tls, s, sslhost) != 0) {
> + if (tls_connect_socket(tls, fd, sslhost) != 0) {
> fprintf(ttyout, "SSL failure: %s\n", tls_error(tls));
> goto cleanup_url_get;
> }
> } else {
> - fin = fdopen(s, "r+");
> - s = -1;
> + fin = fdopen(fd, "r+");
> + fd = -1;
> }
> #else /* !NOSSL */
> - fin = fdopen(s, "r+");
> - s = -1;
> + fin = fdopen(fd, "r+");
> + fd = -1;
> #endif /* !NOSSL */
>
> #ifdef SMALL
> @@ -918,9 +918,9 @@ noslash:
> fclose(fin);
> fin = NULL;
> }
> - if (s != -1) {
> - close(s);
> - s = -1;
> + if (fd != -1) {
> + close(fd);
> + fd = -1;
> }
> rval = url_get(redirurl, proxyenv, savefile, lastfile);
> free(redirurl);
> @@ -1049,9 +1049,9 @@ cleanup_url_get:
> fclose(fin);
> fin = NULL;
> }
> - if (s != -1) {
> - close(s);
> - s = -1;
> + if (fd != -1) {
> + close(fd);
> + fd = -1;
> }
> if (out >= 0 && out != fileno(stdout))
> close(out);
>
