Skip to content

read_fru_area: fix memset() parameters #191

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

PavelKiryukhin
Copy link
Contributor

Current memset parameters are calculated in the assumption
that we use the buffer for whole FRU. Whereas we use individual
buffers for each area that are smaller than whole FRU size.
That may lead to attempt to memset beyond allocated buffer.
Also: add comment to fail path in fru_area_print_product().

Current memset parameters are calculated in the assumption
that we use the buffer for whole FRU. Whereas we use individual
buffers for each area that are smaller than whole FRU size.
That may lead to attempt to memset beyond allocated buffer.
Also: add comment to fail path in fru_area_print_product().
@yontalcar
Copy link
Contributor

I think it would be more straightforward to zero the whole buffer unconditionally as there are more cases where the buffer won't be filled (https://github.com/ipmitool/ipmitool/blob/master/lib/ipmi_fru.c#L777). Or drop the memset call there as the buffer should be zeroed by calling function.

Also the same buggy code is in read_fru_area_section().

@AlexanderAmelkin
Copy link
Contributor

The whole fru support is rusty, buggy, spaghetti-like and generally awful. It needs a complete re-write or at least a major refactoring. I've been thinking about it for a long time already. Thought of maybe expanding the frugen libfru library and using it, thought of other approaches, but I still don't have time and courage to start this fight.

These minor fixes may be fine, but they are doing something with all these strange functions that duplicate one another, and use strange names for their arguments and variables, so I am not sure how to review this and be sure that the fixes are correct. When I start doing so, I immediately rediscover how awful the fru code is and feel the urge to rewrite it all, but... (see above).

Hopefully one day soon I will find time to rewrite it all, or maybe some Heracles appears to help me clean these Augean stables of fru... Until then I think I will not be accepting fixes to fru. Sorry.

@yontalcar
Copy link
Contributor

@AlexanderAmelkin
I agree that the code is ugly and confusing, but this is a regression from IPMITOOL_1_8_18 introduced in commit e824c23 .

So I guess something should be done about it.

@pcahyna
Copy link

pcahyna commented Feb 3, 2021

@PavelKiryukhin is there a reproducer to show some problematic behavior? While the code does not look correct, ipmitool fru print seems to behave as it should.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants