On Thu, Jul 1, 2021 at 8:38 PM Christian Göttsche <cgzones@xxxxxxxxxxxxxx> wrote: > > Unsigned integer overflow is well-defined and not undefined behavior. > But it is still useful to enable undefined behavior sanitizer checks on > unsigned arithmetic to detect possible issues on counters or variables > with similar purpose. > > Use a spaceship operator like comparison instead of subtraction. > > Modern compilers will generate a single comparison instruction instead > of actually perform the subtraction. > > policydb.c:851:24: runtime error: unsigned integer overflow: 801 - 929 cannot be represented in type 'unsigned int' > > This is similar to 1537ea84. While I agree with the change, I still see subtractions in gcc 11.1 and clang 12. What do you call "modern compilers"? More precisely, I copied the function rangetr_cmp in Godbolt and compiled it with "x86-64 gcc 11.1" and -O2, https://godbolt.org/z/5c4jG7eeh . The generated assembly code contains: mov eax, DWORD PTR [rsi] cmp DWORD PTR [rdi], eax seta al movzx eax, al sbb eax, 0 test eax, eax jne .L1 This really computes a subtraction, with instruction SBB. For people not familiar with x86_64 assembly language: * the first two instructions compare two values, * SETA sets the 8-bit register AL to 1 if the first value was above the other one, * MOVZX ensures that the 32-bit register EAX contains 0 or 1, * SBB computes the subtraction between EAX and the carry flag, which is one only if the first value was below the other one. * TEST checks whether the result of the subtraction is zero. For information, the previous code generated the following instructions: mov eax, DWORD PTR [rdi] sub eax, DWORD PTR [rsi] jne .L1 So the previous code generated simpler assembler instructions, but which could trigger an undefined behavior in C. TL;DR Anyway, as this change is not about performance but about undefined behaviour in C, it looks good to me. Nevertheless the sentence "Modern compilers will generate a single comparison instruction instead of actually perform the subtraction." seems to be wrong. Could you rephrase it (for example by giving examples of such compilers), or remove it? Thanks, Nicolas PS: about the integer overflow itself, I read https://wiki.sei.cmu.edu/confluence/display/c/INT30-C.+Ensure+that+unsigned+integer+operations+do+not+wrap too, which seems to be clear that subtracting two unsigned integers and expecting the subtraction to wrap is something which should be avoided. > Signed-off-by: Christian Göttsche <cgzones@xxxxxxxxxxxxxx> > --- > libsepol/src/policydb.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c > index ef2217c2..5ee78b4c 100644 > --- a/libsepol/src/policydb.c > +++ b/libsepol/src/policydb.c > @@ -843,15 +843,15 @@ static int rangetr_cmp(hashtab_t h __attribute__ ((unused)), > const struct range_trans *key2 = (const struct range_trans *)k2; > int v; > > - v = key1->source_type - key2->source_type; > + v = (key1->source_type > key2->source_type) - (key1->source_type < key2->source_type); > if (v) > return v; > > - v = key1->target_type - key2->target_type; > + v = (key1->target_type > key2->target_type) - (key1->target_type < key2->target_type); > if (v) > return v; > > - v = key1->target_class - key2->target_class; > + v = (key1->target_class > key2->target_class) - (key1->target_class < key2->target_class); > > return v; > } > -- > 2.32.0 >
