NetBSD Problem Report #59587

From root@GW.SoftJAR.SE  Mon Aug 11 12:29:06 2025
Return-Path: <root@GW.SoftJAR.SE>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
	 key-exchange X25519 server-signature RSA-PSS (2048 bits)
	 client-signature RSA-PSS (2048 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id EF7D51A923A
	for <gnats-bugs@gnats.NetBSD.org>; Mon, 11 Aug 2025 12:29:05 +0000 (UTC)
Message-Id: <20250811115433.DBB1F38FCF1@GW.SoftJAR.SE>
Date: Mon, 11 Aug 2025 13:54:33 +0200 (CEST)
From: bqt@softjar.se
Reply-To: bqt@softjar.se
To: gnats-bugs@NetBSD.org
Subject: ftp client bug
X-Send-Pr-Version: 3.95

>Number:         59587
>Category:       bin
>Synopsis:       ftp client ascii transfers with progress report fail
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    lukem
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon Aug 11 12:30:00 +0000 2025
>Closed-Date:    Thu Jan 22 22:06:15 +0000 2026
>Last-Modified:  Thu Jan 22 22:06:15 +0000 2026
>Originator:     Johnny Billquist
>Release:        NetBSD 10.1
>Organization:
N/A
>Environment:
System: NetBSD GW.SoftJAR.SE 10.1 NetBSD 10.1 (GENERIC) #0: Wed Feb 5 23:06:02 CET 2025 root@GW.SoftJAR.SE:/usr/obj/sys/arch/amd64/compile/GENERIC amd64
Architecture: x86_64
Machine: amd64
>Description:
A couple of years ago, the ftp (client) was changed in how it handled
signals (version 1.171).

The problem is that the change made reads from the data channel
potentially cause EINTR or EAGAIN. The code was changed to properly
handle this in binary transfers, but it was not done for ascii
transfers. So if anyone tries to transfer a large enough ascii
file, with a progress bar, the transfer will fail after about 1s.

>How-To-Repeat:
ftp to some server, switch to ascii mode, make sure progress is enabled,
and try to fetch a file that is large enough that it will take more than
a couple of seconds to transfer the file, and observe the error aborting
the transfer.

>Fix:

Here is a patch to fix the problem:

Index: ftp.c
===================================================================
RCS file: /cvsroot/src/usr.bin/ftp/ftp.c,v
retrieving revision 1.174.2.3
diff -r1.174.2.3 ftp.c
1125c1125,1136
< 		while ((c = getc(din)) != EOF) {
---
> 		while (1) {
> 			c = getc(din);
> 			if (c == EOF) {
> 				if (feof(din)) {
> 					goto break2;
> 				}
> 				if ((errno == EINTR) || (errno == EAGAIN)) {
> 					clearerr(din);
> 					goto contin2;
> 				}
> 				goto break2;
> 			}

>Release-Note:

>Audit-Trail:

Responsible-Changed-From-To: bin-bug-people->lukem
Responsible-Changed-By: lukem@NetBSD.org
Responsible-Changed-When: Sat, 06 Dec 2025 06:11:22 +0000
Responsible-Changed-Why:
I'm handling it


State-Changed-From-To: open->feedback
State-Changed-By: lukem@NetBSD.org
State-Changed-When: Sat, 06 Dec 2025 06:23:00 +0000
State-Changed-Why:
I think I've fixed this in -current, ftp version 20251206.
I provide ftp_getc() and ftp_putc() wrappers that handle EINTR/EAGAIN
and adapt the control and data channel code to use those
(inspired by your patch which address this in one code path).
If you can confirm this fixes the issue for you and doesn't introduce
any obvious other regressions, I'll ask for pullups to -11 and -10


From: "Luke Mewburn" <lukem@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/59587 CVS commit: src/usr.bin/ftp
Date: Sat, 6 Dec 2025 06:20:23 +0000

 Module Name:	src
 Committed By:	lukem
 Date:		Sat Dec  6 06:20:23 UTC 2025

 Modified Files:
 	src/usr.bin/ftp: extern.h ftp.c util.c version.h

 Log Message:
 ftp: fix ascii transfers with progress bar

 Handle stdio interruption by signals and improve error handling
 in getc() and putc() on the control and data channels.
 Provide ftp_getc() and ftp_putc() wrappers that:
 - Retry the operation on EINTR or EAGAIN instead of failing.
 - Store other error codes in a return variable separate to errno,
   and use that variable in ferror() handling, for more correct
   error messages.

 Fixes the progress bar display in ascii mode transfers.

 (Note that I haven't fixed interrupted reads from stdin or
 writes to ttyout; that's a much larger refactor that's out of scope).

 Fix PR bin/59587 from Johnny Billquist, with the fix
 inspired by the patch in that PR.


 To generate a diff of this commit:
 cvs rdiff -u -r1.84 -r1.85 src/usr.bin/ftp/extern.h
 cvs rdiff -u -r1.178 -r1.179 src/usr.bin/ftp/ftp.c
 cvs rdiff -u -r1.168 -r1.169 src/usr.bin/ftp/util.c
 cvs rdiff -u -r1.100 -r1.101 src/usr.bin/ftp/version.h

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: Johnny Billquist <bqt@softjar.se>
To: gnats-bugs@gnats.netbsd.org
Cc: 
Subject: Re: bin/59587
Date: Tue, 13 Jan 2026 18:39:26 +0100

 Thanks. Looks good to me.

    Johnny

State-Changed-From-To: feedback->closed
State-Changed-By: gutteridge@NetBSD.org
State-Changed-When: Tue, 13 Jan 2026 21:54:56 +0000
State-Changed-Why:
Submitter is satisfied with the change, closing. Thanks for the PR!

State-Changed-From-To: closed->pending-pullups
State-Changed-By: lukem@NetBSD.org
State-Changed-When: Wed, 14 Jan 2026 07:35:19 +0000
State-Changed-Why:
requesting pullups to netbsd-11 and netbsd-10


Responsible-Changed-From-To: lukem->releng
Responsible-Changed-By: lukem@NetBSD.org
Responsible-Changed-When: Thu, 15 Jan 2026 01:43:06 +0000
Responsible-Changed-Why:
releng pullups submitted


Responsible-Changed-From-To: releng->lukem
Responsible-Changed-By: lukem@NetBSD.org
Responsible-Changed-When: Thu, 15 Jan 2026 01:43:52 +0000
Responsible-Changed-Why:
I'll track the pullup status, rather than making it releng's problem.
pullups are:
- netbsd-11: [pullup-11 #152]
- netbsd-10: [pullup-10 #1222]


From: Taylor R Campbell <riastradh@NetBSD.org>
To: Luke Mewburn <lukem@NetBSD.org>
Cc: source-changes-d@NetBSD.org,
	gnats-bugs@NetBSD.org, netbsd-bugs@NetBSD.org,
	Johnny Billquist <bqt@softjar.se>
Subject: Re: bin/59587: ftp client ascii transfers with progress report fail
Date: Thu, 15 Jan 2026 03:51:28 +0000

 > Module Name:    src
 > Committed By:   lukem
 > Date:           Sat Dec  6 06:20:23 UTC 2025
 > 
 > Modified Files:
 >         src/usr.bin/ftp: extern.h ftp.c util.c version.h
 > 
 > Log Message:
 > ftp: fix ascii transfers with progress bar
 > 
 > Handle stdio interruption by signals and improve error handling
 > in getc() and putc() on the control and data channels.
 > Provide ftp_getc() and ftp_putc() wrappers that:
 > - Retry the operation on EINTR or EAGAIN instead of failing.
 > [...]
 > +	while ((res = getc(fin)) == EOF) {
 > +		if (feof(fin))
 > +			break;		/* return EOF */
 > +		if (ferror(fin)) {
 > +			if ((errno == EINTR) || (errno == EAGAIN)) {
 > +					/* retry on EINTR or EAGAIN */
 > +				clearerr(fin);
 > +				continue;

 Why do you loop on EAGAIN?

 Is the underlying file descriptor non-blocking?

 => If it is non-blocking, then you presumably need to wait in
    select/poll for input to arrive -- otherwise this is a busy-wait.

 => If it isn't non-blocking, then you should never get EAGAIN here --
    EAGAIN is generally only for non-blocking I/O calls to report that
    they can't perform the requested operation until you wait in
    select/poll for something to become ready (some input data to
    become ready or some output data to be processed from the buffer).

 Or is there a bug somewhere that causes some syscall or library
 routine involved to return EAGAIN on a signal, when it should really
 either restart or return EINTR?  (I have never seen such a bug.)

From: Taylor R Campbell <riastradh@NetBSD.org>
To: Luke Mewburn <lukem@NetBSD.org>
Cc: source-changes-d@NetBSD.org,
	gnats-bugs@NetBSD.org, netbsd-bugs@NetBSD.org,
	Johnny Billquist <bqt@softjar.se>
Subject: Re: bin/59587: ftp client ascii transfers with progress report fail
Date: Thu, 15 Jan 2026 04:19:49 +0000

 > Date: Thu, 15 Jan 2026 03:51:28 +0000
 > From: Taylor R Campbell <riastradh@NetBSD.org>
 > 
 > > Module Name:    src
 > > Committed By:   lukem
 > > Date:           Sat Dec  6 06:20:23 UTC 2025
 > > 
 > > Modified Files:
 > >         src/usr.bin/ftp: extern.h ftp.c util.c version.h
 > > 
 > > Log Message:
 > > ftp: fix ascii transfers with progress bar
 > > 
 > > Handle stdio interruption by signals and improve error handling
 > > in getc() and putc() on the control and data channels.
 > > Provide ftp_getc() and ftp_putc() wrappers that:
 > > - Retry the operation on EINTR or EAGAIN instead of failing.

 Alternatively: Why not use SA_RESTART, so you don't have to track down
 all the I/O logic that might be interrupted by a signal and arrange to
 run it in a loop?

 There's nothing here that behaves differently if it received a signal,
 like printing a progress bar if the I/O was interrupted by a signal --
 that's already done in the signal handler itself, isn't it?

 (Except you should probably use snprintf_ss, not snprintf, in
 progressbar.c, if you want to print the progress bar in the signal
 handler itself: snprintf is technically not async-signal-safe.  In
 practice on NetBSD, though, I think it matters only for the
 floating-point formatters, which may use malloc in gdtoa; the rest of
 it like %d and %s is probably safe.)

From: Luke Mewburn <luke@mewburn.net>
To: Taylor R Campbell <riastradh@netbsd.org>
Cc: Luke Mewburn <lukem@netbsd.org>, source-changes-d@netbsd.org,
	gnats-bugs@netbsd.org, netbsd-bugs@netbsd.org,
	Johnny Billquist <bqt@softjar.se>
Subject: Re: bin/59587: ftp client ascii transfers with progress report fail
Date: Thu, 15 Jan 2026 16:34:48 +1100

 On 26-01-15 04:19, Taylor R Campbell wrote:
   | > From: Taylor R Campbell <riastradh@NetBSD.org>
   | > 
   | > > Module Name:    src
   | > > Committed By:   lukem
   | > > Date:           Sat Dec  6 06:20:23 UTC 2025
   | > > 
   | > > Modified Files:
   | > >         src/usr.bin/ftp: extern.h ftp.c util.c version.h
   | > > 
   | > > Log Message:
   | > > ftp: fix ascii transfers with progress bar
   | > > 
   | > > Handle stdio interruption by signals and improve error handling
   | > > in getc() and putc() on the control and data channels.
   | > > Provide ftp_getc() and ftp_putc() wrappers that:
   | > > - Retry the operation on EINTR or EAGAIN instead of failing.
   | 
   | Alternatively: Why not use SA_RESTART, so you don't have to track down
   | all the I/O logic that might be interrupted by a signal and arrange to
   | run it in a loop?

 Even on NetBSD, the previous use of restartable signals (SA_RESTART)
 caused issues with the implementation of -q QUITTIME.
 That's why I changed ftp 20210106 to always use interruptable
 signals for PR 55857 - as confirmed by the submitter.


   | There's nothing here that behaves differently if it received a signal,
   | like printing a progress bar if the I/O was interrupted by a signal --
   | that's already done in the signal handler itself, isn't it?
   | 
   | (Except you should probably use snprintf_ss, not snprintf, in
   | progressbar.c, if you want to print the progress bar in the signal
   | handler itself: snprintf is technically not async-signal-safe.  In
   | practice on NetBSD, though, I think it matters only for the
   | floating-point formatters, which may use malloc in gdtoa; the rest of
   | it like %d and %s is probably safe.)

From: Luke Mewburn <luke@mewburn.net>
To: Taylor R Campbell <riastradh@netbsd.org>
Cc: Luke Mewburn <lukem@netbsd.org>, source-changes-d@netbsd.org,
	gnats-bugs@netbsd.org, netbsd-bugs@netbsd.org,
	Johnny Billquist <bqt@softjar.se>
Subject: Re: bin/59587: ftp client ascii transfers with progress report fail
Date: Thu, 15 Jan 2026 16:26:16 +1100

 On 26-01-15 03:51, Taylor R Campbell wrote:
   | > Module Name:    src
   | > Committed By:   lukem
   | > Date:           Sat Dec  6 06:20:23 UTC 2025
   | > 
   | > Modified Files:
   | >         src/usr.bin/ftp: extern.h ftp.c util.c version.h
   | > 
   | > Log Message:
   | > ftp: fix ascii transfers with progress bar
   | > 
   | > Handle stdio interruption by signals and improve error handling
   | > in getc() and putc() on the control and data channels.
   | > Provide ftp_getc() and ftp_putc() wrappers that:
   | > - Retry the operation on EINTR or EAGAIN instead of failing.
   | > [...]
   | > +	while ((res = getc(fin)) == EOF) {
   | > +		if (feof(fin))
   | > +			break;		/* return EOF */
   | > +		if (ferror(fin)) {
   | > +			if ((errno == EINTR) || (errno == EAGAIN)) {
   | > +					/* retry on EINTR or EAGAIN */
   | > +				clearerr(fin);
   | > +				continue;
   | 
   | Why do you loop on EAGAIN?
   | 
   | Is the underlying file descriptor non-blocking?
   | 
   | => If it is non-blocking, then you presumably need to wait in
   |    select/poll for input to arrive -- otherwise this is a busy-wait.
   | 
   | => If it isn't non-blocking, then you should never get EAGAIN here --
   |    EAGAIN is generally only for non-blocking I/O calls to report that
   |    they can't perform the requested operation until you wait in
   |    select/poll for something to become ready (some input data to
   |    become ready or some output data to be processed from the buffer).
   | 
   | Or is there a bug somewhere that causes some syscall or library
   | routine involved to return EAGAIN on a signal, when it should really
   | either restart or return EINTR?  (I have never seen such a bug.)

 I was following the pattern used previously in the ftp code for
 handling interrupted raw I/O on the data and control sessions,
 which may be on non-blocking descriptors.
 For purity purposes I /could/ remove the EAGAIN checks, but I think
 it makes it easier when eyeball checking the differences in
 I/O handling across the code base for raw vs stdio.


 To be frank, whilst a fix removing all use of non-signal-safe stdio
 with ftp-specific code for the line protocol handling (FTP control,
 HTTP headers) might be "better", I don't think it's worth the effort.
 This works.

 ftp as a protocol is on life support.  Whilst I've spent many years
 augmenting this tool, it's a lost cause. HTTP "won". Other tools exist.
 So aiming for perfection is not going to happen.
 The whole code base started on 1980s grad student C code with too many
 globals, setjmp, etc, and modified by 1990s grad students (me),
 and others since.  It's not how anyone would write code today.

From: Taylor R Campbell <riastradh@NetBSD.org>
To: Luke Mewburn <luke@mewburn.net>
Cc: source-changes-d@NetBSD.org,
	gnats-bugs@NetBSD.org, netbsd-bugs@NetBSD.org,
	Johnny Billquist <bqt@softjar.se>
Subject: Re: bin/59587: ftp client ascii transfers with progress report fail
Date: Thu, 15 Jan 2026 23:56:10 +0000

 > Date: Thu, 15 Jan 2026 16:34:48 +1100
 > From: Luke Mewburn <luke@mewburn.net>
 > 
 > On 26-01-15 04:19, Taylor R Campbell wrote:
 >   | Alternatively: Why not use SA_RESTART, so you don't have to track down
 >   | all the I/O logic that might be interrupted by a signal and arrange to
 >   | run it in a loop?
 > 
 > Even on NetBSD, the previous use of restartable signals (SA_RESTART)
 > caused issues with the implementation of -q QUITTIME.
 > That's why I changed ftp 20210106 to always use interruptable
 > signals for PR 55857 - as confirmed by the submitter.

 Sounds like a good reason, thanks!  I hadn't looked at any surrounding
 context, where there is a loop that depends on re-reading the time
 after an alarm expires while waiting in read/recv/accept.

From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/59587 CVS commit: [netbsd-11] src/usr.bin/ftp
Date: Thu, 22 Jan 2026 20:00:00 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Thu Jan 22 20:00:00 UTC 2026

 Modified Files:
 	src/usr.bin/ftp [netbsd-11]: extern.h ftp.c util.c version.h

 Log Message:
 Pull up following revision(s) (requested by lukem in ticket #152):

 	usr.bin/ftp/ftp.c: revision 1.179
 	usr.bin/ftp/version.h: revision 1.101
 	usr.bin/ftp/util.c: revision 1.169
 	usr.bin/ftp/extern.h: revision 1.85

 ftp: fix ascii transfers with progress bar

 Handle stdio interruption by signals and improve error handling
 in getc() and putc() on the control and data channels.

 Provide ftp_getc() and ftp_putc() wrappers that:
 - Retry the operation on EINTR or EAGAIN instead of failing.
 - Store other error codes in a return variable separate to errno,
   and use that variable in ferror() handling, for more correct
   error messages.

 Fixes the progress bar display in ascii mode transfers.
 (Note that I haven't fixed interrupted reads from stdin or
 writes to ttyout; that's a much larger refactor that's out of scope).

 Fix PR bin/59587 from Johnny Billquist, with the fix
 inspired by the patch in that PR.


 To generate a diff of this commit:
 cvs rdiff -u -r1.84 -r1.84.2.1 src/usr.bin/ftp/extern.h
 cvs rdiff -u -r1.178 -r1.178.2.1 src/usr.bin/ftp/ftp.c
 cvs rdiff -u -r1.168 -r1.168.2.1 src/usr.bin/ftp/util.c
 cvs rdiff -u -r1.99.2.1 -r1.99.2.2 src/usr.bin/ftp/version.h

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/59587 CVS commit: [netbsd-10] src/usr.bin/ftp
Date: Thu, 22 Jan 2026 20:01:33 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Thu Jan 22 20:01:32 UTC 2026

 Modified Files:
 	src/usr.bin/ftp [netbsd-10]: extern.h ftp.c util.c version.h

 Log Message:
 Pull up following revision(s) (requested by lukem in ticket #1222):

 	usr.bin/ftp/ftp.c: revision 1.179
 	usr.bin/ftp/version.h: revision 1.101
 	usr.bin/ftp/util.c: revision 1.169
 	usr.bin/ftp/extern.h: revision 1.85

 ftp: fix ascii transfers with progress bar

 Handle stdio interruption by signals and improve error handling
 in getc() and putc() on the control and data channels.

 Provide ftp_getc() and ftp_putc() wrappers that:
 - Retry the operation on EINTR or EAGAIN instead of failing.
 - Store other error codes in a return variable separate to errno,
   and use that variable in ferror() handling, for more correct
   error messages.

 Fixes the progress bar display in ascii mode transfers.
 (Note that I haven't fixed interrupted reads from stdin or
 writes to ttyout; that's a much larger refactor that's out of scope).

 Fix PR bin/59587 from Johnny Billquist, with the fix
 inspired by the patch in that PR.


 To generate a diff of this commit:
 cvs rdiff -u -r1.82.10.2 -r1.82.10.3 src/usr.bin/ftp/extern.h
 cvs rdiff -u -r1.174.2.3 -r1.174.2.4 src/usr.bin/ftp/ftp.c
 cvs rdiff -u -r1.164.2.4 -r1.164.2.5 src/usr.bin/ftp/util.c
 cvs rdiff -u -r1.95.2.4 -r1.95.2.5 src/usr.bin/ftp/version.h

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: pending-pullups->closed
State-Changed-By: lukem@NetBSD.org
State-Changed-When: Thu, 22 Jan 2026 22:06:15 +0000
State-Changed-Why:
Fix has been pulled up to netbsd-10 and netbsd-11


>Unformatted:

Home	PR Database Search