11587 loader.efi: comparison is always true due to limited range of data type
Review Request #2241 — Created Aug. 20, 2019 and submitted
Information | |
---|---|
tsoome | |
illumos-gate | |
11587 | |
6c4b798... | |
Reviewers | |
general | |
../copy.c: In function 'efi_copyin': ../copy.c:209:14: error: comparison is always true due to limited range of data type [-Werror=type-limits] 209 | assert(dest < 0x100000000); | ^ ../../../../../include/assert.h:54:21: note: in definition of macro 'assert' 54 | #define assert(e) ((e) ? (void)0 : __assert(__func__, __FILE__, \ | ^ ../copy.c: In function 'efi_copyout': ../copy.c:217:13: error: comparison is always true due to limited range of data type [-Werror=type-limits] 217 | assert(src < 0x100000000); | ^ ../../../../../include/assert.h:54:21: note: in definition of macro 'assert' 54 | #define assert(e) ((e) ? (void)0 : __assert(__func__, __FILE__, \ | ^ cc1: all warnings being treated as errors
-
-
usr/src/boot/sys/boot/efi/loader/copy.c (Diff revision 1) Is it legal for dest to be UINT32_MAX? I'm just noticing that we used to check for an invalid value, so maybe that means it should be <=? I'm not sure if that'll still end up causing problems for the 32-bit build or not.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+6 -6) |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+6 -6) |
-
-
usr/src/boot/sys/boot/efi/loader/copy.c (Diff revision 3) I'm assuming we don't care about overflow possibilities in the debug assert?
Change Summary:
add overflow check
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+6 -6) |
-
-
usr/src/boot/sys/boot/efi/loader/copy.c (Diff revision 4) In libi386,
i386_copyin()
,i386_copyout()
(andi386_readin()
) uses regularif (...)
checks.
Why the EFI implementation uses DEGUG-onlyassert()
instead? -
usr/src/boot/sys/boot/efi/loader/copy.c (Diff revision 4) Missing range check?
Compared to libi386::i386_readin() function,
efi_readin()
passing it's arguments toread()
without any checks.
Why?
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+24 -12) |
Change Summary:
I think I had the condition reversed:D
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+24 -12) |
Change Summary:
for 32-bit build, we need to cast to 64-bit data type to have meaningful test.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+24 -12) |
-
-
usr/src/boot/sys/boot/efi/loader/copy.c (Diff revision 7) Why do we cast dest in some cases but not others?
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 8 (+24 -12) |