Re: git: b88df1e893c4 - main - Reapply "sbin/ping: allow normal users to specify larger packets"

Hi Pedro,

No, this is not right.  Let me clarify:

(1) I never told that there are any issues with the tests.  I just
mumbled that the tests should catch such regression though I never
checked if they actually did.

(2) The MAXPAYLOAD calculation in the code below is not fully correct.

It should be

65535 - 20 (ip header) - 8 (icmp part) = 65507 without IP options

OR

65535 - 20 (ip header) - 40 (ip options) - 8 (icmp part) = 65467 with
IP options, ie. whenever you run ping -R.

The code below hardcoded the latter value which is simply wrong.

I wouldn't rely on the fact that you get it from other BSD flavours
and would recommend to have this code reviewed before committing it.

Maxim

On Wed, 16 Oct 2024, 18:40-0000, Pedro F. Giffuni wrote:

> The branch main has been updated by pfg:
>
> URL: https://cgit.FreeBSD.org/src/commit/?id=b88df1e893c455731c7915f72a3b97b078ab04e2
>
> commit b88df1e893c455731c7915f72a3b97b078ab04e2
> Author:     Pedro F. Giffuni <pfg@FreeBSD.org>
> AuthorDate: 2024-10-16 18:35:44 +0000
> Commit:     Pedro F. Giffuni <pfg@FreeBSD.org>
> CommitDate: 2024-10-16 18:39:48 +0000
>
>     Reapply "sbin/ping: allow normal users to specify larger packets"
>
>     The ping tests were originally broken by an unrelated isue that
>     is now fixed ( 2926c2594249f64fecbbdcb0b0b9a3591931ab04 ).
>
>     THanks to kp@ for fixing the test and Jose Luis Duran for pinting it out.
>
>     This reverts commit 7bc0cb91a2dfc7e23d96efd0fb7866ee2a23ba88.
> ---
>  sbin/ping/ping.8 |  3 +--
>  sbin/ping/ping.c | 11 +++++------
>  2 files changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/sbin/ping/ping.8 b/sbin/ping/ping.8
> index 0eaec196e1e3..951049d0f252 100644
> --- a/sbin/ping/ping.8
> +++ b/sbin/ping/ping.8
> @@ -25,7 +25,7 @@
>  .\" OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
>  .\" SUCH DAMAGE.
>  .\"
> -.Dd September 15, 2023
> +.Dd October 15, 2024
>  .Dt PING 8
>  .Os
>  .Sh NAME
> @@ -312,7 +312,6 @@ with the 8 bytes of
>  ICMP
>  header data.
>  .Pp
> -For IPv4, only the super-user may specify values more than default.
>  This option cannot be used with ping sweeps.
>  .Pp
>  For IPv6, you may need to specify
> diff --git a/sbin/ping/ping.c b/sbin/ping/ping.c
> index d9d544bc75c8..e6b1247af497 100644
> --- a/sbin/ping/ping.c
> +++ b/sbin/ping/ping.c
> @@ -96,8 +96,8 @@
>  #define	DEFDATALEN	56		/* default data length */
>  #define	FLOOD_BACKOFF	20000		/* usecs to back off if F_FLOOD mode */
>  					/* runs out of buffer space */
> -#define	MAXIPLEN	(sizeof(struct ip) + MAX_IPOPTLEN)
> -#define	MAXICMPLEN	(ICMP_ADVLENMIN + MAX_IPOPTLEN)
> +#define	MAXIPLEN	((int)sizeof(struct ip) + MAX_IPOPTLEN)
> +#define	MAXPAYLOAD	(IP_MAXPACKET - MAXIPLEN - ICMP_MINLEN)
>  #define	MAXWAIT		10000		/* max ms to wait for response */
>  #define	MAXALARM	(60 * 60)	/* max seconds for alarm timeout */
>  #define	MAXTOS		255
> @@ -458,11 +458,10 @@ ping(int argc, char *const *argv)
>  				errx(EX_USAGE, "invalid packet size: `%s'",
>  				    optarg);
>  			datalen = (int)ltmp;
> -			if (uid != 0 && datalen > DEFDATALEN) {
> -				errno = EPERM;
> -				err(EX_NOPERM,
> +			if (datalen > MAXPAYLOAD) {
> +				errx(EX_USAGE,
>  				    "packet size too large: %d > %u",
> -				    datalen, DEFDATALEN);
> +				    datalen, MAXPAYLOAD);
>  			}
>  			break;
>  		case 'T':		/* multicast TTL */
>

-- 
Maxim Konovalov