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

[Christian Mauderer]

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?

> 
> 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.

> 
>     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.

> 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>
>      >
>