[PATCH 2/4] bsps/shared/ofw: Use memcpy instead of strncpy

[Gedare Bloom]

On Sat, Feb 6, 2021 at 1:59 AM Christian Mauderer <oss at c-mauderer.de> wrote:

> On 06/02/2021 06:02, Niteesh G. S. wrote:
> > Hello Christian,
> >
> > On Sat, Feb 6, 2021 at 2:33 AM Christian Mauderer <oss at c-mauderer.de
> > <mailto:oss at c-mauderer.de>> wrote:
> >
> >     On 05/02/2021 19:22, Gedare Bloom wrote:
> >      >
> >      >
> >      > On Fri, Feb 5, 2021 at 10:41 AM G S Niteesh Babu
> >     <niteesh.gs at gmail.com <mailto:niteesh.gs at gmail.com>
> >      > <mailto:niteesh.gs at gmail.com <mailto:niteesh.gs at gmail.com>>>
> wrote:
> >      >
> >      >     Changed rtems_ofw_get_prop to use memcpy instead of strncpy
> >      >     ---
> >      >       bsps/shared/ofw/ofw.c | 10 +++++++++-
> >      >       1 file changed, 9 insertions(+), 1 deletion(-)
> >      >
> >      >     diff --git a/bsps/shared/ofw/ofw.c b/bsps/shared/ofw/ofw.c
> >      >     index fa94bfbf05..9dec310247 100644
> >      >     --- a/bsps/shared/ofw/ofw.c
> >      >     +++ b/bsps/shared/ofw/ofw.c
> >      >     @@ -198,7 +198,15 @@ ssize_t rtems_ofw_get_prop(
> >      >
> >      >         if (prop == NULL && strcmp(propname, "name") == 0) {
> >      >           prop = fdt_get_name(fdtp, offset, &len);
> >      >     -    strncpy(buf, prop, bufsize);
> >      >     +
> >      >     +    bufsize = MIN(len, bufsize - 1);
> >      >
> >      > ok, reserving 1 byte for the \0. It could be worth adding a
> >     comment here
> >      > to that effect
> >
> >     Is the content of that property really _allways_ a string? Isn't it
> >     possible to read some references or similar with it?
> >
> > Yes in this case it is always a string since we check if the propname ==
> > "name"
> > In other cases we use bcopy. I wanted to get this to your attention but
> > then I
> > confused myself with endianness and thought it will still be an issue.
>
> Sorry, I think that confusion has been caused by me. If the spec tells
> that "name" is always a string and can never be something else, keeping
> str*cpy is OK from my point of view. What do you think Gedare?
>
> Yes


> >
> > And as per the device tree spec, the node name is 1-31 chars length,
> > consists
> > only of ASCII chars(Device tree spec table 2.1) and is null-terminated.
>
> In that case enforcing null-termination might still be a good idea in
> case the buffer passed to this function is smaller or in case someone
> passes an invalid device tree.
>
> +1

And add a comment like you said about the spec.


> >
> >     If it is always a string, I might have made a useless suggestion. In
> >     that case it might is more efficient and readable to just keep the
> >     strncpy. Depending on the use case, maybe using strlcpy instead of
> >     strncpy could be a good idea to guarantee the \0 termination.
> >
> > As per docs, strlcpy  is  not  present  in glibc and is not standardized
> > by POSIX,
> > so is it okay to use that?
>
> As far as I know, newlib (the C library we use) has a strlcpy. It is
> already used in some rtems code (for example
> cpukit/score/src/threadname.c) so I don't see a problem using it.
>
>
We assume it exists inside of rtems.


> > If not I can use strncpy with a check like this
> >
> >     strncpy(buf, str, buflen - 1);
> >
> >             if (buflen > 0)
> >                 buf[buflen - 1]= '\0';
> > This ensures the buffer is always null terminated.
> >
> >      >
> >      >     +    memcpy(buf, prop, bufsize);
> >      >     +
> >      >     +    /* Null terminate the buffer */
> >      >     +    *((char *)buf + bufsize) = 0;
> >      >
> >      > that gets written here. looks fine to me.
> >      >
> >      >     +
> >      >     +    /* Return the length of the name including the null byte
> */
> >      >     +    /* This is the behaviour in libBSD ofw_fdt_getprop */
> >      >           return len + 1;
> >      >
> >      > shouldn't it be bufsize+1? if it got truncated by the MIN?
> >      >
> >      >         }
> >      >
> >      >     --
> >      >     2.17.1
> >      >
> >      >
> >      > _______________________________________________
> >      > devel mailing list
> >      > devel at rtems.org <mailto:devel at rtems.org>
> >      > http://lists.rtems.org/mailman/listinfo/devel
> >     <http://lists.rtems.org/mailman/listinfo/devel>
> >      >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20210206/e645bc2e/attachment.html>