linux-kernel - Re: [PATCH 2/4] cpupower: Remove redundant error check

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]

Message-ID: <CA+MoWDqriZojBJAaxSjjFXEf1rpxDLqtsiuYYsRqDwtY1L6QyA@mail.gmail.com>
Date:	Sun, 18 May 2014 00:31:29 +0200
From:	Peter Senna Tschudin <peter.senna@...il.com>
To:	Dan Carpenter <dan.carpenter@...cle.com>
Cc:	Dominik Brodowski <linux@...inikbrodowski.net>,
	Thomas Renninger <trenn@...e.de>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
	Alan Cox <alan@...ux.intel.com>,
	kernel-janitors@...r.kernel.org
Subject: Re: [PATCH 2/4] cpupower: Remove redundant error check

On Sat, May 17, 2014 at 11:56 PM, Dan Carpenter
<dan.carpenter@...cle.com> wrote:
> On Sat, May 17, 2014 at 11:34:46PM +0200, Peter Senna Tschudin wrote:
>> On Sat, May 17, 2014 at 10:22 PM, Dan Carpenter
>> <dan.carpenter@...cle.com> wrote:
>> > On Sat, May 17, 2014 at 08:22:58PM +0200, Peter Senna Tschudin wrote:
>> >> diff --git a/tools/power/cpupower/utils/cpufreq-set.c b/tools/power/cpupower/utils/cpufreq-set.c
>> >> index a416de8..4e2f35a 100644
>> >> --- a/tools/power/cpupower/utils/cpufreq-set.c
>> >> +++ b/tools/power/cpupower/utils/cpufreq-set.c
>> >> @@ -320,12 +320,11 @@ int cmd_freq_set(int argc, char **argv)
>> >>
>> >>               printf(_("Setting cpu: %d\n"), cpu);
>> >>               ret = do_one_cpu(cpu, &new_pol, freq, policychange);
>> >> -             if (ret)
>> >> +             if (ret) {
>> >> +                     print_error();
>> >>                       break;
>> >
>> > Just return directly instead of break return;
>> >
>> >> +             }
>> >>       }
>> >>
>> >> -     if (ret)
>> >> -             print_error();
>> >> -
>> >>       return ret;
>> >
>> > Are you sure this patch is correct?  Theoretically, it's possible to
>> > reach the end of this function without going hitting the
>> > "ret = do_one_cpu(...);" assignment.
>> >
>> > Don't be fooled by the "int ret = 0;" initialization, that is a trick
>> > initialization to mislead the unwary.  By the end of the do while loop
>> > then "ret" is always -1.
>> I have missed that, thank you for pointing this out. This patch is
>> wrong and should not be applied, please ignore it.
>>
>> Dan, should I just leave this file as it is?
>
> I think in reality we should always hit the "ret = do_one_cpu()"
> assignment.  But your static analysis tool should say that we don't know
> that, so that's why I brought it up.
>
> My guess is that the original code is bad and we should say:
>
>                 ret = do_one_cpu(cpu, &new_pol, freq, policychange);
>                 if (ret) {
>                         print_error();
>                         return ret;
>                 }
>         }
>
>         return 0;
Sent V2. Thank you for the help.

>
> I am currently involved in a number of threads, not just yours, where I
> am encouraging people to replace ambiguous returns with "return 0;".
> This is my life now.
So maybe you like this list of 160 places in which the return variable
is initialized and only used as parameter to return(The list look
good, but I haven't reviewed all 160, so there could be problems):
http://pastebin.com/5kAbCP2e

Does it worth doing something about those trivial cases?

Do you have more examples of ambiguous returns, so I can help you hunt them?




>
> regards,
> dan carpenter
>



-- 
Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/