From: Ingo Schwarze Subject: better error reporting in ftp(1) auto_fetch() To: tech@openbsd.org Cc: "H. Hartzer" Date: Mon, 5 May 2025 01:23:16 +0200 Hello, H. Hartzer wrote on Sun, May 04, 2025 at 03:14:52AM +0000: > I personally like when programs are explicit about being used > incorrectly. I strongly agree. A program ought to return a non-zero exit status when used incorrectly. In particular if it is intended for interactive use, it should usually also print an error message to stderr. > if (strchr(argv[argpos], ':') == NULL) > + fprintf(ttyout, "%s isn't a valid URL.\n", argv[argpos]); > break; Henrich has a point here. This function reports most errors in a meaningful way, but there are two exceptions where control sneaks out of the function silently, failing to report the problem to the user. The above patch is quite wrong though: * Error messages are destined for stderr, not for ttyout. * Using the warn(3)/err(3) family of functions is preferred over using fprintf(3) for multiple reasons, both in general and by existing precedent in this particular file. * For want of curlies, the above insertion moves the "break;" statement out of the condition, resulting in the whole function no longer doing any work whatsoever. I audited the function with respect to error handling, resulting in the following patch. Rationale: * This function handles all synopses listed in the ftp(1) manual except the first one. * The "for" loop appearing in the patch is executed once for each URL argument (ftp:// http[s]:// file: host:path) * Note that even though the four URL formats are quite varied, each of them contains a colon. That's what the check in the first chunk verifies. * The first chunk does not only lack the printed error message, but also neglects setting the exit status, resulting in a spurious report of success: $ /oldbin/ftp http://bsd.lv/index.html nocolon ; echo $? Trying 66.111.2.12... Requesting http://bsd.lv/index.html 100% |**************************************************| 5220 00:0 5220 bytes received in 0.00 seconds (33.08 MB/s) 0 $ ftp http://bsd.lv/index.html nocolon ; echo $? Trying 66.111.2.12... Requesting http://bsd.lv/index.html 100% |**************************************************| 5220 00:00 5220 bytes received in 0.00 seconds (33.27 MB/s) ftp: No colon in URL: nocolon 2 * The "break" statement in the first chunk is not wrong, but *all* the other error exits in this function use "continue;", and after setting rval, we can do that here, too, simply for uniformity. * The EMPTYSTRING test in the second chunk is ineffective for being premature. The string "host" only becomes empty once the directory is cut off, hence we have to move the EMPTYSTRING test down by a few lines. $ /oldbin/ftp ftp:///FOO ; echo $? ftp: : no address associated with name ftp: Can't connect or login to host `' 1 $ ftp ftp:///FOO ; echo $? ftp: No host name in URL: ftp:///FOO 1 $ /oldbin/ftp http://bsd.lv/index.html :FOO ; echo $? Trying 66.111.2.12... Requesting http://bsd.lv/index.html 100% |**************************************************| 5220 00:00 5220 bytes received in 0.00 seconds (8.60 MB/s) ftp: : no address associated with name ftp: Can't connect or login to host `' 2 $ ftp http://bsd.lv/index.html :FOO ; echo $? Trying 66.111.2.12... Requesting http://bsd.lv/index.html 100% |**************************************************| 5220 00:00 5220 bytes received in 0.00 seconds (23.20 MB/s) ftp: No host name in URL: :FOO 2 Code growth is 49480 -> 49632 for fetch.o on amd64 (+152 bytes). I did not test install media. Is this already OK, or should more testing be done? Ingo Index: fetch.c =================================================================== RCS file: /cvs/src/usr.bin/ftp/fetch.c,v diff -U4 -p -r1.218 fetch.c --- fetch.c 23 Apr 2024 08:50:38 -0000 1.218 +++ fetch.c 4 May 2025 23:01:06 -0000 @@ -1274,10 +1274,13 @@ auto_fetch(int argc, char *argv[], char * Loop through as long as there's files to fetch. */ username = pass = NULL; for (rval = 0; (rval == 0) && (argpos < argc); free(url), argpos++) { - if (strchr(argv[argpos], ':') == NULL) - break; + if (strchr(argv[argpos], ':') == NULL) { + warnx("No colon in URL: %s", argv[argpos]); + rval = argpos + 1; + continue; + } free(username); free(pass); host = dir = file = portnum = username = pass = NULL; @@ -1399,19 +1402,21 @@ bad_ftp_url: portnum = NULL; } else { /* classic style `host:file' */ dir = strchr(host, ':'); } - if (EMPTYSTRING(host)) { - rval = argpos + 1; - continue; - } /* * If dir is NULL, the file wasn't specified * (URL looked something like ftp://host) */ if (dir != NULL) *dir++ = '\0'; + + if (EMPTYSTRING(host)) { + warnx("No host name in URL: %s", argv[argpos]); + rval = argpos + 1; + continue; + } /* * Extract the file and (if present) directory name. */