Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38421 )
Change subject: commonlib/bsd: Add new CBFS core implementation ......................................................................
Patch Set 15:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38421/15/src/commonlib/bsd/cbfs_pri... File src/commonlib/bsd/cbfs_private.c:
https://review.coreboot.org/c/coreboot/+/38421/15/src/commonlib/bsd/cbfs_pri... PS15, Line 119: assert(todo <= sizeof(*dst) - already_read);
I didn't look too closely, but if so then it's bad
This was the discussion from https://review.coreboot.org/c/coreboot/+/38421/11/src/commonlib/bsd/cbfs_pri.... This function is only intended to be called with the arguments that cbfs_walk() passed out to a walker() callback and if used that way, it should be guaranteed to be safe. Of course if you call a function with random garbage and ignore the intention explained in the docstring, bad things may happen. But this whole API is designed so that you can only really get access to cbfs_mdata structs that have passed the basic checks in cbfs_walk().
Running into this would signify a severe programming error so I think assert() is the appropriate construct. It's not here for input validation. (FWIW I wouldn't mind turning FATAL_ASSERTS on by default everywhere but that's a completly separate topic.)