Messages in this thread Patch in this message | | | Date | Tue, 5 Aug 2014 16:24:24 -0400 | From | Chris Metcalf <> | Subject | Re: [PATCH] arch: tile: gxio: mpipe.c: Cleaning up missing null-terminate in conjunction with strncpy |
| |
On 7/26/2014 10:03 AM, Rickard Strandqvist wrote: > Replacing strncpy with strlcpy to avoid strings that lacks null terminate. > > Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se> > ---
While using strlcpy() instead of strncpy() and an explicit NUL termination is obviously an improvement, on reflection it doesn't feel like the best solution here - and likely in many similar APIs. A partial string argument should not be passed on to other components; it's just an error by definition and should be treated that way. While we could check the strlcpy() return value explicitly against the length, this is somewhat awkward as it requires naming the length twice, and also means we end up doing the copy pointlessly if we're about to return an error anyway.
The following change includes Yet Another String Copy function; this may or may not be a fabulous idea, but at least it encapsulates the error checking in a way that seems like it makes the call sites cleaner.
From: Chris Metcalf <cmetcalf@tilera.com> Date: Tue, 5 Aug 2014 16:16:13 -0400 Subject: [PATCH] tile: avoid errors from truncating long strings in mpipe gxio
Using strncpy() will just silently truncate long strings; we should instead return an appropriate error. Using strlcpy() would suffer from the same problem. Instead, use strlen()+memcpy(), and add an error-checking step to make sure the lengths are reasonable.
I called the convenience wrapper strscpy(), and a case could be made for making it more generic (possibly with a better name), but that seems outside the scope of this initial commit.
Signed-off-by: Chris Metcalf <cmetcalf@tilera.com> --- arch/tile/gxio/mpipe.c | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-)
diff --git a/arch/tile/gxio/mpipe.c b/arch/tile/gxio/mpipe.c index 5301a9ffbae1..8b3c201dfdb7 100644 --- a/arch/tile/gxio/mpipe.c +++ b/arch/tile/gxio/mpipe.c @@ -29,6 +29,25 @@ /* HACK: Avoid pointless "shadow" warnings. */ #define link link_shadow
+/* + * Use this routine to avoid copying too-long strings. Unlike strncpy + * or strlcpy, we don't enable programmers who don't check return codes; + * partially-copied strings can be problematic. The routine returns + * the total number of bytes copied (including the trailing NUL) or + * zero if the buffer wasn't big enough. + */ +static size_t strscpy(char *dest, const char *src, size_t size) +{ + size_t ret = strlen(src) + 1; + if (ret > size) { + if (size) + dest[0] = '\0'; + return 0; + } + memcpy(dest, src, ret); + return ret; +} + int gxio_mpipe_init(gxio_mpipe_context_t *context, unsigned int mpipe_index) { char file[32]; @@ -511,8 +530,8 @@ int gxio_mpipe_link_instance(const char *link_name) if (!context) return GXIO_ERR_NO_DEVICE;
- strncpy(name.name, link_name, sizeof(name.name)); - name.name[GXIO_MPIPE_LINK_NAME_LEN - 1] = '\0'; + if (strscpy(name.name, link_name, sizeof(name.name)) == 0) + return GXIO_ERR_NO_DEVICE;
return gxio_mpipe_info_instance_aux(context, name); } @@ -529,7 +548,8 @@ int gxio_mpipe_link_enumerate_mac(int idx, char *link_name, uint8_t *link_mac)
rv = gxio_mpipe_info_enumerate_aux(context, idx, &name, &mac); if (rv >= 0) { - strncpy(link_name, name.name, sizeof(name.name)); + if (strscpy(link_name, name.name, sizeof(name.name)) == 0) + return GXIO_ERR_INVAL_MEMORY_SIZE; memcpy(link_mac, mac.mac, sizeof(mac.mac)); }
@@ -545,8 +565,8 @@ int gxio_mpipe_link_open(gxio_mpipe_link_t *link, _gxio_mpipe_link_name_t name; int rv;
- strncpy(name.name, link_name, sizeof(name.name)); - name.name[GXIO_MPIPE_LINK_NAME_LEN - 1] = '\0'; + if (strscpy(name.name, link_name, sizeof(name.name)) == 0) + return GXIO_ERR_NO_DEVICE;
rv = gxio_mpipe_link_open_aux(context, name, flags); if (rv < 0) -- 1.8.3.1 -- Chris Metcalf, Tilera Corp. http://www.tilera.com
| |