[PATCH 2/4] bsps/shared/ofw: Use memcpy instead of strncpy
Christian Mauderer
oss at c-mauderer.de
Sat Feb 6 08:59:50 UTC 2021
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>
> >
>
More information about the devel
mailing list