linux-kernel - Re: [PATCH] powercap: Adjust printing the constraint name with new line

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 17 Nov 2020 13:51:02 +0100
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Lukasz Luba <lukasz.luba@....com>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux PM <linux-pm@...r.kernel.org>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>
Subject: Re: [PATCH] powercap: Adjust printing the constraint name with new line

On Tue, Nov 17, 2020 at 10:22 AM Lukasz Luba <lukasz.luba@....com> wrote:
>
> Hi Rafael,
>
> On 11/10/20 7:19 PM, Rafael J. Wysocki wrote:
> > On Mon, Nov 9, 2020 at 6:25 PM Lukasz Luba <lukasz.luba@....com> wrote:
> >>
> >> The constraint name has limit of size 30, which sometimes might be hit.
> >> When this happens the new line might be lost. Prevent this and set the
> >> new line when the name string is too long."
> >>
> >> Signed-off-by: Lukasz Luba <lukasz.luba@....com>
> >> ---
> >>   drivers/powercap/powercap_sys.c | 4 ++++
> >>   1 file changed, 4 insertions(+)
> >>
> >> diff --git a/drivers/powercap/powercap_sys.c b/drivers/powercap/powercap_sys.c
> >> index f808c5fa9838..575f9fdb810e 100644
> >> --- a/drivers/powercap/powercap_sys.c
> >> +++ b/drivers/powercap/powercap_sys.c
> >> @@ -174,6 +174,10 @@ static ssize_t show_constraint_name(struct device *dev,
> >>                                                                  "%s\n", name);
> >>                          buf[POWERCAP_CONSTRAINT_NAME_LEN] = '\0';
> >>                          len = strlen(buf);
> >> +
> >> +                       /* When the 'name' is too long, don't lose new line */
> >> +                       if (strlen(name) >= POWERCAP_CONSTRAINT_NAME_LEN)
> >> +                               buf[POWERCAP_CONSTRAINT_NAME_LEN - 1] = '\n';
> >
> > Wouldn't it be better to pass POWERCAP_CONSTRAINT_NAME_LEN - 1 to
> > snprintf() above?
>
> My apologies for delay, I was on holidays.
>
> The snprintf() overwrites the '\n' in that case also.

The snprintf() doesn't do that AFAICS, but the next assignment
overwrites the newline with the zero character.

Anyway, it should have been POWERCAP_CONSTRAINT_NAME_LEN - 2, because
it needs to leave space for 2 characters, the newline and the
terminating zero.

> I've experimented with it a bit more and tried to come with a bit 'cleaner' solution.
>
> What we are looking is probably: "%.*s\n" semantic.
> Another solution would be:
> ------------------------------8<---------------------------
>                  if (name) {
> -                       snprintf(buf, POWERCAP_CONSTRAINT_NAME_LEN,
> -                                                               "%s\n",
> name);
> -                       buf[POWERCAP_CONSTRAINT_NAME_LEN] = '\0';
> +                       sprintf(buf, "%.*s\n",
> POWERCAP_CONSTRAINT_NAME_LEN -1,
> +                               name);
>                          len = strlen(buf);
>                  }
>
> ----------------------------->8----------------------------
>
> I've check this and it behaves very well.
>
> It looks cleaner and it is a used pattern in the kernel.
> What do you think? Is it good for v2?

This works for me too.

Thanks!