From nobody Mon Jun 30 15:19:08 2025 X-Original-To: dev-commits-src-main@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 4bW8vz65msz60Zwk for ; Mon, 30 Jun 2025 15:19:27 +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 4bW8vz4MYzz3MTs for ; Mon, 30 Jun 2025 15:19:27 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-pg1-x533.google.com with SMTP id 41be03b00d2f7-879d2e419b9so3484306a12.2 for ; Mon, 30 Jun 2025 08:19:27 -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=gRGX3dA9+h0+jbjd7oHe3TXbChrQP8UrBnn2d2dzhyw2Evdn93jDKhHb9yOMVOIQ6U 2V7ckvCRs66tbw7wMg4vsHpLQn6fTlisOuNC4IzUN63ipXJl4t/ScFAFILskGtwCnZrg kdz2Lm0cTMtskurMz6/5oryJtWBOimJvk9mx0119+v7HG8IIV/aywYJ9i9ztpSmWKQHT afv6+cz+8Du8iMmmAnbzeUIWSZ77zygilNn5LRP8kmhlPkEXR8nFttvllFV7J3Fvbo5E y/Nd4F9ctpFKnhpG10m107OZimPuL2ic29aH9qxH+UQQ3X6ADO6QZBjTErq/ia1Exou4 p6mg== X-Forwarded-Encrypted: i=1; AJvYcCV7JaehFiqxuDgEyvOZWzSxOzl52pQPXV40M/MtnGCo3ZFFE0MX1VXDWOYzO76LrXynaYpw6rkmHvZswXmBmIvjZDBuGg==@freebsd.org X-Gm-Message-State: AOJu0Yz+uWv+qLOw5/kvVl188iY3FclVz3fF9kDBBLYZdmRVl3QC3MxK TdFORlq56F71UL5uIHYmLDMAZ+gnehgkBywKadR0XSAtNqeQ/BBSHFGlkKec7s1zn7gVEN+hrRV 8hA8E0+YkxuZcnka4TolakHfcKRlZDE0PyzAIAvIUww== X-Gm-Gg: ASbGncsTrb8fGr6eP6hncwvX+f+r9Dsftdyu21fzqxrHtD538pxCWzWHx0djWTPqQvy 8Oyg66C3p4CXeglxzQNSG2x8dxRJ+EURnfEW0C3NCXtthwguIqih8P4/Ri+nXSdwU9GRRv17trb lJq71gk2ZzS4zHXEPE4wKODvjmH0FMJIdmYDzkVfWpQS1ecpG01ZztRw== 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 the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-main@freebsd.org Sender: owner-dev-commits-src-main@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: 4bW8vz4MYzz3MTs 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