Attention is currently required from: Jérémy Compostella, Martin L Roth, Maximilian Brune.
4 comments:
File src/commonlib/include/commonlib/region.h:
Patch Set #1, Line 103: static inline int region_create_untrusted(
Does it make sense to mix the constructor and the checking of invalid input values? […]
Certainly possible but we would have to repeat the check for different
callers. And that this mustn't overflow is an implementation detail,
kind of the contract (pre-condition) of region_create() with the assert()
in place. Though, we have no way to enforce such contracts in C.
Patch Set #1, Line 104: struct region *r, unsigned long long offset, unsigned long long size)
Is there a specific reason why you use "unsigned long long" type?
It's the biggest there is. The idea is to ensure that values are not
accidentally truncated when calling the function. The _unstrusted
function should work 100%ly no matter what the caller looks like.
Patch Set #1, Line 106: offset > SIZE_MAX || size > SIZE_MAX
I am puzzled. […]
size_t/SIZE_MAX are implementation defined. I'm rather sure that compiled
for x86_32 size_t is 32 bits wide. Still, somebody would call the function
with a value >= 4G. (C compilers don't warn about implicit truncation, and
static analyzers often only do so when they have reason to believe that
the value is too big.)
File util/cbfstool/cbfstool.c:
Patch Set #1, Line 334: if (region_create_untrusted(
If I understand correctly the only real difference between `region_create` and `region_create_untrus […]
These regions can come from the command line. It's a minor difference with
the default behavior of assert(). Though, if I'd have a typo in my command line,
I'd prefer a proper error message over an annoying "assertion failed".
And generally, seeing command-line input as unstrusted seems right IMO.
To view, visit change 79905. To unsubscribe, or for help writing mail filters, visit settings.