-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add off-by-one mutation #26
Conversation
As suggeested on llogiq#5, it would be nice to mutate numeric constants adding or substracting one unit. At the moment, this is very conservative on the limits to add or substract on default integer constants. I've did it like this to avoid compilation issue, as we do not have information regarding the type of the expression. So, in the default case, it subtracts one only if it's > 0 and it only adds one if the value is < 256. If the constant has a type annotation (for example, 0i32), I adjust the minimum and the maximum depending on the type hint. Note that it does not check u128, as it's only available on nightly. Finally, it chains the mutations in order to record only the needed mutations. So, if the code finds a "0" on an unsigned type, it will only record the "add one" mutation, but not the "sub one".
plugin/src/lib.rs
Outdated
fn int_constant_can_add_one(i: u64, ty: LitIntType) -> bool { | ||
let max: u64 = match ty { | ||
LitIntType::Unsuffixed => 256, | ||
LitIntType::Unsigned(UintTy::Usize) => std::u32::MAX as u64, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I doubt which should be the max value for usize. As far as I know, it's system dependent, so I'm not sure which is the most safe value here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be a problem as long as we're consistent (that is, usize::max()
is the right value on all targets)
plugin/src/lib.rs
Outdated
|
||
fn int_constant_can_add_one(i: u64, ty: LitIntType) -> bool { | ||
let max: u64 = match ty { | ||
LitIntType::Unsuffixed => 256, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I also have doubts here. Maybe I should change it to std::i8::MAX, as it's the max positive value that won't cause compilation issues?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, let's be careful here.
Great work so far! I was just thinking about implementing this mutation and you beat me to it 😄 Apart from small nits, this looks good to merge to me. If you like, feel free to add unit tests, but I won't block on them now. |
I've been checking this again and I've found that it does not properly work with negative values.
I get: |
This is because you have an UnOp(UnNeg, Lit(..)) – working around will require folding UnOps and looking into whether the contained |
@gnieto Do you want to extend this or should I merge as is and do it myself? I'll be happy in either case. |
If you don't mind, i would like to finish it. I expect to finish it late
today or tomorrow with the negative literal handling. Thanks!
El dia 25 febr. 2018 15:10, "llogiq" <notifications@github.com> va escriure:
@gnieto <https://github.com/gnieto> Do you want to extend this or should I
merge as is and do it myself? I'll be happy in either case.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#26 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AEbE8FnepcBUUBWZNCqIwQnj1ipwqMLLks5tYWnKgaJpZM4SRdvG>
.
|
Great! No offense meant. I just wanted to make sure the PR doesn't become stale. 👍 |
Handling for negative literals added 😃 |
As Rust AST does not track negative literals on ExprKind::Lit, the code has been changed in order to check for Unary operations which contains a negative operand and then the literal. The code has been factored in order to allow to entry points: one for negative literals and one for positive literals.
01c74d2
to
ed98cb3
Compare
Great work! Thanks again! 👍 |
As suggested on #5, it would be
nice to mutate numeric constants adding or subtracting one unit to numeric constants.
At the moment, this is very conservative on the limits to add or
substract on default integer constants. I've did it like this to avoid
compilation issue, as we do not have information regarding the type of
the expression. So, in the default case, it subtracts one only if it's >
0 and it only adds one if the value is < 256.
If the constant has a type annotation (for example, 0i32), It adjust the
minimum and the maximum depending on the type hint. Note that it does
not check 128 bits numbers, as they are only available on nightly.
It chains the mutations in order to record only the needed
mutations. So, if the code finds a "0" on an unsigned type, it will only
record the "add one" mutation, but not the "sub one" (as it would overflow and panic).
After running it on the example project, it emits mutations like:
or
And finally: I didn't add tests. I thought on adding another test on the example project that covers some of the edge cases and I've also thought on unit test the
int_constant_can_subtract_one
orint_constant_can_add_one
, but I'm not sure which is the expected way of testing this library, so, if you have some idea, I can add the kind of tests you think will be more useful.