Re: [sudeep-holla:for-next/scmi 23/30] drivers/firmware/arm_scmi/driver.c:2531 scmi_debugfs_common_setup() error: uninitialized symbol 'c_ptr'.

[Cristian Marussi <cristian.marussi@arm.com>]

On Thu, Jan 05, 2023 at 12:16:34PM +0300, Dan Carpenter wrote:
> On Thu, Jan 05, 2023 at 08:26:30AM +0000, Cristian Marussi wrote:
> > > vim +/c_ptr +2531 drivers/firmware/arm_scmi/driver.c
> > > 
> > > 0868ff68bdf60d Cristian Marussi 2022-12-29  2512  static struct scmi_debug_info *scmi_debugfs_common_setup(struct scmi_info *info)
> > > 0868ff68bdf60d Cristian Marussi 2022-12-29  2513  {
> > > 0868ff68bdf60d Cristian Marussi 2022-12-29  2514  	int ret;
> > > 0868ff68bdf60d Cristian Marussi 2022-12-29  2515  	const char *c_ptr;
> > > 0868ff68bdf60d Cristian Marussi 2022-12-29  2516  	char top_dir[16];
> > > 0868ff68bdf60d Cristian Marussi 2022-12-29  2517  	struct dentry *trans, *top_dentry;
> > > 0868ff68bdf60d Cristian Marussi 2022-12-29  2518  	struct scmi_debug_info *dbg;
> > > 0868ff68bdf60d Cristian Marussi 2022-12-29  2519  
> > > 0868ff68bdf60d Cristian Marussi 2022-12-29  2520  	dbg = devm_kzalloc(info->dev, sizeof(*dbg), GFP_KERNEL);
> > > 0868ff68bdf60d Cristian Marussi 2022-12-29  2521  	if (!dbg)
> > > 0868ff68bdf60d Cristian Marussi 2022-12-29  2522  		return NULL;
> > > 0868ff68bdf60d Cristian Marussi 2022-12-29  2523  
> > > 0868ff68bdf60d Cristian Marussi 2022-12-29  2524  	dbg->name = kstrdup(of_node_full_name(info->dev->of_node), GFP_KERNEL);
> > > 0868ff68bdf60d Cristian Marussi 2022-12-29  2525  	if (!dbg->name) {
> > > 0868ff68bdf60d Cristian Marussi 2022-12-29  2526  		devm_kfree(info->dev, dbg);
> > > 0868ff68bdf60d Cristian Marussi 2022-12-29  2527  		return ERR_PTR(-ENOMEM);
> > > 0868ff68bdf60d Cristian Marussi 2022-12-29  2528  	}
> > > 0868ff68bdf60d Cristian Marussi 2022-12-29  2529  
> > > 0868ff68bdf60d Cristian Marussi 2022-12-29  2530  	of_property_read_string(info->dev->of_node, "compatible", &c_ptr);
> > > 0868ff68bdf60d Cristian Marussi 2022-12-29 @2531  	dbg->type = kstrdup(c_ptr, GFP_KERNEL);
> > > 
> > > My instinct is to agree with the checker on this and say that it does
> > > feel like we should check if of_property_read_string() read something.
> > > 
> > 
> > Well I could have, but I avoided it because this .compatible is related
> > to the device node attached to info->dev which is the platform device
> > itself which is being probed by this driver and from it is exactly from
> > that probe path, indeed, that this same function is called here: so the
> > property cannot be empty or non-existent here since it is the very
> > reason we are in this code path.
> 
> I feel like that requires knowledge outside of just the function?  For
> these emails, I only see the function itself with no context.
> 
> > 
> > ... but, for peace of mind, I could add simply 'c_ptr = NULL;' upfront
> > and let kstrdup trigger the fail path when called with a NULL
> > c_ptr...this indeed has basically no cost and it is more future-proof
> > probably...
> > 
> > Thoughts ?
> 
> Yes.  That sounds good to me.  It likely will not even affect the built
> code for most distro kernels because these days we tend to zero out
> stack data by default to avoid undefine behavior and security issues
> with unintialized data.
> 

Ok, Thanks I'll fix in v7.

Thanks,
Cristian