On Wed, 18 Jul 2018, Daniel Thompson wrote:

On Wed, Jul 18, 2018 at 02:08:53PM +0100, Lee Jones wrote:

quoted

quoted

quoted

quoted

quoted

No, then we are back to the initial issue of num_steps
potentially not
being initialised. We really want both of_property_read_u32() to
succeed AND num_steps to actually be set.

I also think num_steps should be pre-initialised.

Yes, I guess it definitely does not hurt.

quoted

quoted

Then it will only be set if of_property_read_u32() succeeds.

Yes, but we still need to check for both, the function not failing and
num_steps to actually be non zero.

Why?  You don't do anything differently if it fails.

Only if you initialize num_steps...

We should either initialize to zero and not worry about the return
code[1] or we check the return code and not worry about
initialization[2]. I don't think both are worthwhile.

Whilst initialization can fix this specific instance we generally avoid
overusing it since it messes up static analysis and, in this instance,
distance from declaration to use is >25 lines, hence current patchset.


Daniel.


[1] https://lkml.org/lkml/2018/7/16/399
[2] https://lkml.org/lkml/2018/7/16/1042

Or...

We check the return code and leave number

num_steps is uninitialized and stack allocated so it only has a valid
value if of_property_read_u32() succeeds.

We can (and I originally did) fix the bug by initializing num_steps to 0
but its quite some distance between declaration and use so I accepted
Marcel's counter proposal to check the return code instead.

Only checking the return value of of_property_read_u32() is also
suitable.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog