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

[Pavel Cahyna]

On Wed, Feb 21, 2024 at 09:50:35AM -0800, Al Chu wrote:
> Yeah, to verify you'd need a BMC that has the ability to configure a dynamic
> IP address.

I found that I have such a system, actually, so I tried the old and new
(with yout patch below) build.

Old:
# ipmi-config --checkout -e Lan6_Conf:IPv6_Dynamic_Address_Source_Type
Section Lan6_Conf
        ## READ-ONLY
        ## IPv6_Dynamic_Address_Source_Type           
EndSection

# valgrind ipmi-config --checkout -e Lan6_Conf:IPv6_Dynamic_Address_Source_Type
==26033== ERROR SUMMARY: 264 errors from 46 contexts (suppressed: 0 from 0)

New:
# ipmi-config --checkout -e Lan6_Conf:IPv6_Dynamic_Address_Source_Type
Section Lan6_Conf
        ## READ-ONLY
        ## IPv6_Dynamic_Address_Source_Type           SLAAC
EndSection
# valgrind ipmi-config --checkout -e Lan6_Conf:IPv6_Dynamic_Address_Source_Type
==25466== ERROR SUMMARY: 256 errors from 38 contexts (suppressed: 0 from 0)

Certainly looks like an improvement!

Thank you, Pavel

> here's the patch i just commited upstream
> 
> https://github.com/chu11/freeipmi-mirror/commit/41d0d70f09b4becfceef0517543cbf335c0e927a
> 
> Thanks for the catch
> 
> Oh and I got an e-mail that said you subscribed to freeipmi-devel today? 
> I'm not sure what was up with gnu's mailman, but hopefully it's fixed now.
> 
> Al
> 
> On 2/21/24 09:25, Pavel Cahyna wrote:
> > On Wed, Feb 21, 2024 at 09:04:13AM -0800, Al Chu wrote:
> > > 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?
> > I have not hit the bug, I am not using ipmi-config myself, actually. The
> > bug was found by a static code scanner. Do you know how would one
> > reproduce it or in general test this code? I suppose this needs a BMC
> > with an IPv6 address configured?
> > 
> > It is not entirely critical, but I would prefer not to update to a
> > release which has a coding error added. If you have a patch, I will add
> > it to the CentOS Stream and Fedora packages.
> > 
> > Regards, Pavel
> > 
> > > 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
> > > 
> -- 
> Al Chu
> Livermore Computing
> Lawrence Livermore National Laboratory
>