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

[Niteesh G. S.]

Hello Christian,

On Sat, Feb 6, 2021 at 2:33 AM Christian Mauderer <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>> 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.

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.

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?
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
> > http://lists.rtems.org/mailman/listinfo/devel
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20210206/1b2ecb28/attachment.html>