On Fri, Jun 08, 2018 at 04:03:55PM -0400, Steven Rostedt wrote:
> On Thu, 7 Jun 2018 11:18:16 +0200
> Johan Hovold <johan@xxxxxxxxxx> wrote:
>
>
> > If you want to work around the warning and think you can do it in some
> > non-contrived way, then go for it.
> >
> > Clearing the request buffer, checking for termination using strnlen, and
> > then using memcpy might not be too bad.
> >
> > But after all, it is a false positive, so leaving things as they stand
> > is fine too.
>
> Not sure how contrived you think this is, but it solves the warning
> without adding extra work in the normal case.
>
> -- Steve
>
> diff --git a/drivers/staging/greybus/fw-management.c b/drivers/staging/greybus/fw-management.c
> index 71aec14f8181..4fb9f1dff47d 100644
> --- a/drivers/staging/greybus/fw-management.c
> +++ b/drivers/staging/greybus/fw-management.c
> @@ -150,15 +150,18 @@ static int fw_mgmt_load_and_validate_operation(struct fw_mgmt *fw_mgmt,
> }
>
> request.load_method = load_method;
> - strncpy(request.firmware_tag, tag, GB_FIRMWARE_TAG_MAX_SIZE);
> + strncpy(request.firmware_tag, tag, GB_FIRMWARE_TAG_MAX_SIZE - 1);
>
> /*
> * The firmware-tag should be NULL terminated, otherwise throw error and
> * fail.
> */
> - if (request.firmware_tag[GB_FIRMWARE_TAG_MAX_SIZE - 1] != '\0') {
> - dev_err(fw_mgmt->parent, "load-and-validate: firmware-tag is not NULL terminated\n");
> - return -EINVAL;
> + if (request.firmware_tag[GB_FIRMWARE_TAG_MAX_SIZE - 2] != '\0') {
> + if (tag[GB_FIRMWARE_TAG_MAX_SIZE - 1] != '\0') {
> + dev_err(fw_mgmt->parent, "load-and-validate: firmware-tag is not NULL terminated\n");
> + return -EINVAL;
> + }
> + request.firmware_tag[GB_FIRMWARE_TAG_MAX_SIZE - 1] = '\0';
> }
Well, I think it's quite far from obvious what is going on above, and
not least why things are being done this way (which a comment may help
with).
And just NUL-terminating the (automatic) buffer before returning wasn't
enough? Then it may be better to do away with strncpy completely.
But should we really be working around gcc this way? If the
implementation of this new warning isn't smart enough yet, should it not
just be disabled instead?
Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
