Re: [dmidecode] [PATCH] dmioem: Fix segmentation fault in dmi_hp_240_attr()

[Jerry Hoemann]

On Fri, Aug 05, 2022 at 11:57:04AM +0200, Jean Delvare wrote:
> On Fri, 5 Aug 2022 11:45:19 +0200, Jean Delvare wrote:
> > On Thu, 4 Aug 2022 11:29:05 -0600, Jerry Hoemann wrote:
> > > Did you consider adding a check that format is nonzero to pr_attr()
> > > like pr_list_start() does?  
> > 
> > Yes, I considered that first, especially as the other solution
> > duplicates code, which isn't appealing. However I decided against it
> > for semantic reasons.
> 
> Bahhhh, I hit "Send" by accident, sorry.

My concerns are at a lower level.  There appears to be a difference in
interpretation between Linux and Free BSD tool chain on whether passing
a NULL as format is legal.  I did a quick read of the fprintf function
in the Ansi C 99 standard and I'm of the believe that NULL is not legal.
But even if my interpretation is incorrect, there is still a difference
between the two and I am not going to be testing under Free BSD.
So, I'd probably go ahead an proactively change functions like
pr_handle_name, pr_attr, etc., to check format like pr_list_start does
to protect against this type of issue getting hit in the future.


> 
> So the rationale for my decision is that, if there is no value, you
> aren't really printing an attribute. That's really only a header for a
> list to come, exactly as pr_list_start/pr_list_item/pr_list_end are
> meant for. It turns out that so far the lists printed by pr_list_item
> were simple (only values) while what you need here is label+value list
> items, thus the need to extend the concept.
> 
> While it doesn't really make a difference for the current plain text
> output (as shown by the code duplication), it will matter if we ever
> implement alternative output drivers (such as HTML or JSON).
> 
> If you disagree with my approach, I'm open to discussion and ready to
> read your arguments :-)

I'm fine w/ your thinking here.  My concerns are over a different aspect
of the issue we hit.

> 
> Thinking about it some more, it might make sense to actually extend
> pr_list_item() to be more flexible, by accepting an optional label,
> instead of having two separate functions. I'll give it a try and see
> how it looks.
> 
> -- 
> Jean Delvare
> SUSE L3 Support

-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------