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.
 		 */