Date: Mon, 22 Feb 2016 16:54:22 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Benjamin Kaduk <bjkfbsd@gmail.com> Cc: Ian Lepore <ian@freebsd.org>, Sean Bruno <sbruno@freebsd.org>, "src-committers@freebsd.org" <src-committers@freebsd.org>, svn-src-projects@freebsd.org Subject: Re: svn commit: r295812 - projects/mips64-clang/sys/mips/rmi Message-ID: <20160222161248.D887@besplex.bde.org> In-Reply-To: <CAJ5_RoCoJt8kbWvwvpPMYCDzM56qJjzrm54a=EfwesT-1fdz2A@mail.gmail.com> References: <201602191637.u1JGb6lm055074@repo.freebsd.org> <1456066063.1294.40.camel@freebsd.org> <CAJ5_RoCoJt8kbWvwvpPMYCDzM56qJjzrm54a=EfwesT-1fdz2A@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 21 Feb 2016, Benjamin Kaduk wrote: > On Sun, Feb 21, 2016 at 8:47 AM, Ian Lepore <ian@freebsd.org> wrote: > >> On Fri, 2016-02-19 at 16:37 +0000, Sean Bruno wrote: >>> ... >>> Log: >>> Change a static const string to a #define as the strcpy() throws a >>> warn/error with clang. >> >>> /home/sbruno/mips64-clang/sys/mips/rmi/xls_ehci.c:133:25: error: >>> format string is not a string literal (potentially insecure) >>> [-Werror,-Wformat-security] >>> sprintf(sc->sc_vendor, xlr_vendor_desc); >>> >>> Modified: >>> projects/mips64-clang/sys/mips/rmi/xls_ehci.c >>> >>> Modified: projects/mips64-clang/sys/mips/rmi/xls_ehci.c >>> ===================================================================== >>> ========= >>> --- projects/mips64-clang/sys/mips/rmi/xls_ehci.c Fri Feb 19 >>> 15:53:08 2016 (r295811) >>> +++ projects/mips64-clang/sys/mips/rmi/xls_ehci.c Fri Feb 19 >>> 16:37:06 2016 (r295812) >>> @@ -73,7 +73,7 @@ static device_attach_t ehci_xls_attach; >>> static device_detach_t ehci_xls_detach; >>> >>> static const char *xlr_usb_dev_desc = "RMI XLR USB 2.0 controller"; >>> -static const char *xlr_vendor_desc = "RMI Corp"; >>> +#define XLR_VENDOR_DESC "RMI Corp"; >>> >>> static int >>> ehci_xls_probe(device_t self) >>> @@ -130,7 +130,7 @@ ehci_xls_attach(device_t self) >>> device_set_ivars(sc->sc_bus.bdev, &sc->sc_bus); >>> device_set_desc(sc->sc_bus.bdev, xlr_usb_dev_desc); >>> >>> - sprintf(sc->sc_vendor, xlr_vendor_desc); >>> + sprintf(sc->sc_vendor, XLR_VENDOR_DESC); >>> >>> err = bus_setup_intr(self, sc->sc_irq_res, >>> INTR_TYPE_BIO | INTR_MPSAFE, NULL, >> >> Bah. The compiler should understand that a static const char* is >> equivelent to a string literal for the purposes of this warning. This warning is another compiler bug (or rather, enabling it is usually a user error). Disallowing strings that aren't string literals breaks message catalogs. > Is it? The compiler would need to check that nothing else in the file > writes to xlr_vendor_desc before making that conclusion; on the other hand, > if it was char const * const, then that alone would suffice. That would be of low quality too. Just char const [] works, and doesn't require a confusing number of const's and doesn't waste space for a pointer. The compiler could optimize away the pointer after it does the same checks needed to see that the pointer is not written too. It could also see that the pointer is only read once, so it can be removed. Since the string literal is only used once (similarly for the nearby dev_desc), it should probably be written as itself instead of obfuscating it using a pointer or a macro. The variable to hold the string or a pointer to the string might be useful for debugging. However, the compiler is not prevented from optimizing it away. Something like volatile or or __used is required for that. The macro version ensures that the variable doesn't exist. The string literal might also be optimized away by encoding it. E.g., short string literals of length <= 7 should probably be copied moves of 32-bit data. >> That said, a sprintf() is just a strange spelling of strlcpy() here. > > Almost. sprintf() is not as good about length checking as strlcpy(), which > is a much better option here, as you note. No, it is a bad spelling of strcpy(). The buffer should be large enough to hold the string, and this is very easy to arrange with a literal string. If you write the strcpy() with a string literal (or an array) for the source, and an array for the target, then the compiler can check that the string fits. It should also do that for strlcpy() and snprintf() with "%s", and emit warnings about bogus use of strlcpy() for strings that are sure to fit, and about not checking the return value for strings that aren't sure to fit. sprintf(9) is declared as __printflike(2, 3) and that is apparently enough to enable most warnings about the printf() family. strcpy(9) and strlcpy(9) are declared without any attributes, so any compiler checking of them would be a bug. I don't know of any attributes for them. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160222161248.D887>