<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, Feb 6, 2021 at 1:59 AM Christian Mauderer <<a href="mailto:oss@c-mauderer.de">oss@c-mauderer.de</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 06/02/2021 06:02, Niteesh G. S. wrote:<br>
> Hello Christian,<br>
> <br>
> On Sat, Feb 6, 2021 at 2:33 AM Christian Mauderer <<a href="mailto:oss@c-mauderer.de" target="_blank">oss@c-mauderer.de</a> <br>
> <mailto:<a href="mailto:oss@c-mauderer.de" target="_blank">oss@c-mauderer.de</a>>> wrote:<br>
> <br>
> On 05/02/2021 19:22, Gedare Bloom wrote:<br>
> ><br>
> ><br>
> > On Fri, Feb 5, 2021 at 10:41 AM G S Niteesh Babu<br>
> <<a href="mailto:niteesh.gs@gmail.com" target="_blank">niteesh.gs@gmail.com</a> <mailto:<a href="mailto:niteesh.gs@gmail.com" target="_blank">niteesh.gs@gmail.com</a>><br>
> > <mailto:<a href="mailto:niteesh.gs@gmail.com" target="_blank">niteesh.gs@gmail.com</a> <mailto:<a href="mailto:niteesh.gs@gmail.com" target="_blank">niteesh.gs@gmail.com</a>>>> wrote:<br>
> ><br>
> > Changed rtems_ofw_get_prop to use memcpy instead of strncpy<br>
> > ---<br>
> > bsps/shared/ofw/ofw.c | 10 +++++++++-<br>
> > 1 file changed, 9 insertions(+), 1 deletion(-)<br>
> ><br>
> > diff --git a/bsps/shared/ofw/ofw.c b/bsps/shared/ofw/ofw.c<br>
> > index fa94bfbf05..9dec310247 100644<br>
> > --- a/bsps/shared/ofw/ofw.c<br>
> > +++ b/bsps/shared/ofw/ofw.c<br>
> > @@ -198,7 +198,15 @@ ssize_t rtems_ofw_get_prop(<br>
> ><br>
> > if (prop == NULL && strcmp(propname, "name") == 0) {<br>
> > prop = fdt_get_name(fdtp, offset, &len);<br>
> > - strncpy(buf, prop, bufsize);<br>
> > +<br>
> > + bufsize = MIN(len, bufsize - 1);<br>
> ><br>
> > ok, reserving 1 byte for the \0. It could be worth adding a<br>
> comment here<br>
> > to that effect<br>
> <br>
> Is the content of that property really _allways_ a string? Isn't it<br>
> possible to read some references or similar with it?<br>
> <br>
> Yes in this case it is always a string since we check if the propname == <br>
> "name"<br>
> In other cases we use bcopy. I wanted to get this to your attention but <br>
> then I<br>
> confused myself with endianness and thought it will still be an issue.<br>
<br>
Sorry, I think that confusion has been caused by me. If the spec tells <br>
that "name" is always a string and can never be something else, keeping <br>
str*cpy is OK from my point of view. What do you think Gedare?<br>
<br></blockquote><div>Yes</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> <br>
> And as per the device tree spec, the node name is 1-31 chars length, <br>
> consists<br>
> only of ASCII chars(Device tree spec table 2.1) and is null-terminated.<br>
<br>
In that case enforcing null-termination might still be a good idea in <br>
case the buffer passed to this function is smaller or in case someone <br>
passes an invalid device tree.<br>
<br></blockquote><div>+1</div><div><br></div><div>And add a comment like you said about the spec.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> <br>
> If it is always a string, I might have made a useless suggestion. In<br>
> that case it might is more efficient and readable to just keep the<br>
> strncpy. Depending on the use case, maybe using strlcpy instead of<br>
> strncpy could be a good idea to guarantee the \0 termination.<br>
> <br>
> As per docs, strlcpy is not present in glibc and is not standardized <br>
> by POSIX,<br>
> so is it okay to use that?<br>
<br>
As far as I know, newlib (the C library we use) has a strlcpy. It is <br>
already used in some rtems code (for example <br>
cpukit/score/src/threadname.c) so I don't see a problem using it.<br>
<br></blockquote><div><br></div><div>We assume it exists inside of rtems.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> If not I can use strncpy with a check like this<br>
> <br>
> strncpy(buf, str, buflen - 1);<br>
> <br>
> if (buflen > 0)<br>
> buf[buflen - 1]= '\0';<br>
> This ensures the buffer is always null terminated.<br>
> <br>
> ><br>
> > + memcpy(buf, prop, bufsize);<br>
> > +<br>
> > + /* Null terminate the buffer */<br>
> > + *((char *)buf + bufsize) = 0;<br>
> ><br>
> > that gets written here. looks fine to me.<br>
> ><br>
> > +<br>
> > + /* Return the length of the name including the null byte */<br>
> > + /* This is the behaviour in libBSD ofw_fdt_getprop */<br>
> > return len + 1;<br>
> ><br>
> > shouldn't it be bufsize+1? if it got truncated by the MIN?<br>
> ><br>
> > }<br>
> ><br>
> > --<br>
> > 2.17.1<br>
> ><br>
> ><br>
> > _______________________________________________<br>
> > devel mailing list<br>
> > <a href="mailto:devel@rtems.org" target="_blank">devel@rtems.org</a> <mailto:<a href="mailto:devel@rtems.org" target="_blank">devel@rtems.org</a>><br>
> > <a href="http://lists.rtems.org/mailman/listinfo/devel" rel="noreferrer" target="_blank">http://lists.rtems.org/mailman/listinfo/devel</a><br>
> <<a href="http://lists.rtems.org/mailman/listinfo/devel" rel="noreferrer" target="_blank">http://lists.rtems.org/mailman/listinfo/devel</a>><br>
> ><br>
> <br>
</blockquote></div></div>