From nobody Mon Jun 30 15:19:08 2025 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4bW8vv17Phz60b32 for ; Mon, 30 Jun 2025 15:19:23 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-pg1-x533.google.com (mail-pg1-x533.google.com [IPv6:2607:f8b0:4864:20::533]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "WR4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4bW8vt68zpz3MJx for ; Mon, 30 Jun 2025 15:19:22 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-pg1-x533.google.com with SMTP id 41be03b00d2f7-b2c4331c50eso4513746a12.3 for ; Mon, 30 Jun 2025 08:19:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20230601.gappssmtp.com; s=20230601; t=1751296760; x=1751901560; darn=freebsd.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=4fsmROK+u7UogCOPW4Z11QIMTRmrceRgAlKNxcSsniE=; b=xpHcM27yKKds7mcRj4mks/46of8ZhVRIU7GK2KPbQYUaWVDHxPIGbjFzubpnim+/q/ GI5OMGykXqAu9Sll5oWIYe6fOk4xHcCGosJHfDs/AVWlXFtcoXjoMCNQ+f0Ln4D5duBQ ZYMzBjmaOJ3Zz5yYXULISspm5lhJwlzI+iha0IEYhTiim1BZ78HQU7WoX8Wcdxm+BhEo XYbTCEOwzbTeBIIzCe5HnF85f2XS4twpaPhWV/FauWw/07rv2AvYQ5pios2u7akLFamb 7kQ7YKeU9Ip3wvKNYMXK0LeUkk6dkaB48ry/HblJf972n8JPFOzTcWcrVgQ0CLEgQ2lW T+bg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1751296760; x=1751901560; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=4fsmROK+u7UogCOPW4Z11QIMTRmrceRgAlKNxcSsniE=; b=giPvwHxY575xjVqiP+/qkW6ekoZqG12bBd8wW1RG6Ap8rOD64SgCwdkoSStrLntmdG gj9KOMeJJqrM5Kooe/zbQOVcZ7e6DwAVoI0WYoW/uuqv3Nns4Na5lxiQ0bTPueyoueJ4 aU34XReCIYpU6lOX3ZzTdziYZiIHDim3V2NhhAOKviTpzmtAN21Wqciyz1O7CDF+Mszb SiXCnL66X7Cq2+dxK31BBgsxz9ICtYOSr1d8tc5b4tXeq5okEUEKsW8HS6FfEYP1WHf2 Y2r9bv3bqEvvWF8E2Hd1d8QJFqzcebzK9dOJdTUEwhgdoipKCklUlScmHGCagyg2tZBW ++3A== X-Forwarded-Encrypted: i=1; AJvYcCUiYYO1fXL3j+Ukusx5XnwGG9kc7mQ3wuMnevMTsQ4tpyDyhvMWpa+HokV/Wec5jrIraK/RDpjzvAWJikfxl8K3dWvV@freebsd.org X-Gm-Message-State: AOJu0YyW1XO2fw5xTGlWK1RTkXlSSBT87WLavQA7M/cCA8F+CCJ8NTjA QrLtiaJY3as6sRO7pn3cG2/1184TpR2YxEjEXHZMUfsvZqEPz/4tKhwbEtgjmiAndYvIqx/dzDE qEB5afXOvYIVxiAc1i8b8YYVUKOC7jWBJhQ2NI3BFKGVxCulAh0RERuMllA== X-Gm-Gg: ASbGnctgCjkjJatLIt6zcGb59MyI+akGga0y2AaL3kVpFHwpPQKzU1c70pMhfv8aphD n/7RxriIR79VLhibd5WefeMQt7fERohwOzbd/sBCeDd9eM7QgtMKqFdKh4dkHUojxgzGocyrbTF Cyg7v3d3j0VeKHNG/rOK8KsGjBDBKZucZMwsXcd4tkde0VhTi1UeTEFQ== X-Google-Smtp-Source: AGHT+IH1sMWw0q3I8Kc+KoOJClTF7+tmJMBnS0BE2SSyKNQ9shVQt2lwW8mjrauEcRpUQ7flp5rLDGzxenr7oJdJ4f4= X-Received: by 2002:a17:90b:5251:b0:311:ea13:2e6a with SMTP id 98e67ed59e1d1-318c9225eb1mr18095360a91.13.1751296760163; Mon, 30 Jun 2025 08:19:20 -0700 (PDT) List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-all@freebsd.org Sender: owner-dev-commits-src-all@FreeBSD.org MIME-Version: 1.0 References: <202506270721.55R7LlB1067221@gitrepo.freebsd.org> <69858708-8e73-4b70-9f33-5176fceb1b36@FreeBSD.org> <89e23d9e-885c-4811-886a-6c1011715679@FreeBSD.org> In-Reply-To: <89e23d9e-885c-4811-886a-6c1011715679@FreeBSD.org> From: Warner Losh Date: Mon, 30 Jun 2025 09:19:08 -0600 X-Gm-Features: Ac12FXzz8AtGj1r0IOPA4oyYsXSoHh4-MoiIkzmOgH8u_59ONzbnUPiCPCQkRGc Message-ID: Subject: Re: git: ad8d33679999 - main - mmc_xpt: use strlcpy instead of strncpy To: Andriy Gapon Cc: John Baldwin , src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 4bW8vt68zpz3MJx X-Spamd-Bar: ---- X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US] On Mon, Jun 30, 2025 at 7:57=E2=80=AFAM Andriy Gapon wrot= e: > > On 30/06/2025 16:34, John Baldwin wrote: > > On 6/27/25 03:21, Andriy Gapon wrote: > >> The branch main has been updated by avg: > >> > >> URL: https://cgit.FreeBSD.org/src/commit/? > >> id=3Dad8d33679999c0e7f6fd2b77d2e414102bd365ec > >> > >> commit ad8d33679999c0e7f6fd2b77d2e414102bd365ec > >> Author: Andriy Gapon > >> AuthorDate: 2025-06-23 21:31:04 +0000 > >> Commit: Andriy Gapon > >> CommitDate: 2025-06-27 07:13:34 +0000 > >> > >> mmc_xpt: use strlcpy instead of strncpy > >> A better practice in general. > >> MFC after: 1 week > >> --- > >> sys/cam/mmc/mmc_xpt.c | 6 +++--- > >> 1 file changed, 3 insertions(+), 3 deletions(-) > >> > >> diff --git a/sys/cam/mmc/mmc_xpt.c b/sys/cam/mmc/mmc_xpt.c > >> index 138f96eaaa49..4fce03004994 100644 > >> --- a/sys/cam/mmc/mmc_xpt.c > >> +++ b/sys/cam/mmc/mmc_xpt.c > >> @@ -1213,9 +1213,9 @@ mmc_path_inq(struct ccb_pathinq *cpi, const char= *hba, > >> cpi->max_lun =3D 0; > >> cpi->initiator_id =3D 1; > >> cpi->maxio =3D maxio; > >> - strncpy(cpi->sim_vid, "FreeBSD", SIM_IDLEN); > >> - strncpy(cpi->hba_vid, hba, HBA_IDLEN); > >> - strncpy(cpi->dev_name, cam_sim_name(sim), DEV_IDLEN); > >> + strlcpy(cpi->sim_vid, "FreeBSD", SIM_IDLEN); > >> + strlcpy(cpi->hba_vid, hba, HBA_IDLEN); > >> + strlcpy(cpi->dev_name, cam_sim_name(sim), DEV_IDLEN); > >> cpi->unit_number =3D cam_sim_unit(sim); > >> cpi->bus_id =3D cam_sim_bus(sim); > >> cpi->protocol =3D PROTO_MMCSD; > > > > Hmm, are you sure these aren't depending on strncpy zero-padding > > the result out to the full length? String fields in inquiry/identity > > structures are often not C strings but have other requirements. > > (Some of them are space padded instead of \0 padded for example.) > > Not sure, but using sim_vid as an example, I see strlcpy in cam_xpt and > ctl_frontend_cam_sim. It seems that in context of ccb_pathinq those fiel= ds are > "normal" C strings. You should really get changes like this reviewed. "seems like" is not a good enough reason to do this. In fact, it's a terrible reason in the CAM code. There are places that the strncpy use is intentional and you will screw things up if you use strlcpy. Too many fixed width fields in old-school storage devices that reflected into different APIs, sadly. A better reason that it's 'right' is that all but 3 other places in the tree use strlcpy for this field. The field will be NUL padded, in practice, since CCBs tend to be zero'd before use (though path_inq often is done with stack storage without the usual init). And a quick look shows that we don't use dev_name, hba_vid or sim_vid as a fixed width field anywhere but the 'compat' code that copies the whole thing and doesn't care. We never use any of these three fields in the kernel for anything, apart from filling them in. We do use them in camcontrol (and similar 3rd party programs) to report them to the user as strings. There's other places it looks like they get injected into geom attributes, also C strings. A look at history shows 4195c7de2471d intentionally changed them, and that change was reviewed. It also has the reasons why it's OK in it. So from that perspective, this is a good change. The reasons listed there are sound (though it's too equivocal: these fields were always NUL terminated except when the string was the exact length of the field, which we didn't do anywhere, but that's not important). An even better question is: shouldn't a change like this have checked the other 4 places, and changed them as well? If it's wrong in one place, it might be wrong elsewhere. Or looking elsewhere would confirm or deny that this was intentional for some reason. But there's another consideration that 4195c7de2471d didn't think through that I have just thought of... Warner