Re: Possible uninitialized value since commit 478ffaa7 ipmi-config: correct IPv6 dynamic address checkout error

[Al Chu]

On 2/21/24 03:37, Pavel Cahyna wrote:
> [ please Cc: me in replies, I am not subscribed and my attempt to
> subscribe hits "Bug in Mailman version 2.1.29" .]
>
> Hello Al,
>
> > certain fields are only used for static vs dynamic addresses, so only specific 
> > ones need to be initialized as long as things are programmed correctly.
> >
> > IIRC, the patch below fixed the mistake that I used "source" instead of 
> > "source_type" for dynamic addresses.
> that's my point - the patch below initializes "source_type" and leaves
> "source" uninitialized, but then it keeps using "ipv6_data.source". To
> me, the patch seems incomplete and
>                                                    
> get_dynamic_address_source_type_string (ipv6_data.source)) < 0)
> looks like it should be changed to
>                                                    
> get_dynamic_address_source_type_string (ipv6_data.source_type)) < 0)

Ohhh, I get it now.  I misread the prior e-mail.  Yeah, I think you're 
right, that doesn't look right at all.

Thanks for the catch.  I can get this fixed sometime today.

I assume you hit this bug?  Is it critical for a new release?

Al

> What do you think? If not, I don't see where does ipv6_data.source get
> initialized.
>
> Regards, Pavel
>
> On Tue, Feb 20, 2024 at 05:03:46PM +0100, Pavel Cahyna wrote:
> > Hello,
> >
> > I have a question about this commit:
> >
> > commit 478ffaa7d2ffe240a0afaf9d7d5189342c94bd4d
> > Author: Albert Chu <chu11@llnl.gov>
> > Date:   Wed Jan 26 21:48:47 2022 -0800
> >
> >      ipmi-config: correct IPv6 dynamic address checkout error
> >
> >
> > It contains this change:
> >
> > -----
> > @@ -1089,15 +1090,15 @@ _get_ipv6_dynamic_address (ipmi_config_state_data_t 
> > *state_data,
> >         goto cleanup;
> >       }
> >
> > -  if (FIID_OBJ_GET (obj_cmd_rs, "source", &val) < 0)
> > +  if (FIID_OBJ_GET (obj_cmd_rs, "source_type", &val) < 0)
> >       {
> >         pstdout_fprintf (state_data->pstate,
> >                          stderr,
> > -                       "fiid_obj_get: 'source': %s\n",
> > +                       "fiid_obj_get: 'source_type': %s\n",
> >                          fiid_obj_errormsg (obj_cmd_rs));
> >         goto cleanup;
> >       }
> > -  ipv6_data->source = val;
> > +  ipv6_data->source_type = val;
> >
> >     if (fiid_obj_get_data (obj_cmd_rs,
> >                            "address",
> > -----
> >
> >
> > which, IIUC, leaves ipv6_data->source uninitialized.
> >
> > It then contains this change:
> > -----
> > @@ -1186,7 +1187,7 @@ ipv6_dynamic_address_source_checkout 
> > (ipmi_config_state_data_t *state_data,
> >
> >     if (ipmi_config_section_update_keyvalue_output (state_data,
> >                                                     kv,
> > -                                                  
> > get_dynamic_address_source_string (ipv6_data.source)) < 0)
> > +                                                  
> > get_dynamic_address_source_type_string (ipv6_data.source)) < 0)
> >       return (IPMI_CONFIG_ERR_FATAL_ERROR);
> >
> >     rv = IPMI_CONFIG_ERR_SUCCESS;
> > -----
> >
> > which keeps using ipv6_data.source, which looks uninitialized. This looks 
> > suspect, but I admit I
> > don't really understand the code.
> >
> > Best regards, Pavel

--
Al Chu
Livermore Computing
Lawrence Livermore National Laboratory