-
Notifications
You must be signed in to change notification settings - Fork 3
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
Task 39 fuzzer #57
Task 39 fuzzer #57
Conversation
now shows logs in stderr, for the time being I think its better to leave the other exit codes from mnemoniccli.ExitCode as fails, since they seem more likely to indicate problems with fuzzer than the code. Eg. 'EX_NOINPUT = 66 |
i think the rest of style issues are false positives, as we have to check against several values. If you know of a way to do that dirrectly please let me know |
mnemoniccli -g -e entropy_bin -s seed -m mnemonic -f bin -ll debug &>out
exit_code=$?
if [[ ${exit_code} != 0 && ${exit_code} != 65 ]] ; |
@sobuch, Please add fuzzing of |
this is the same thing as i did just longer, what codeclimate is sugesting is something like |
b295f27
to
04e8b31
Compare
rebased on dev |
radamsa test_seed_hex > seed_hex | ||
|
||
mnemoniccli -g -e entropy_hex -s seed -m mnemonic -ll debug &>out | ||
if [[ $? != 0 && $? != 65 ]] ; then handle_fail ; cp entropy_hex "fail-${fails}" ; fi |
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.
$?
gives you exit code of last command, so it's better to read value once into variable and then do comparison on several places. Variable stays the same, $?
changes if you execute another command in the meantime.
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.
the comparison is done only in one place for each run of mnemoniccli, 'if [[ ... ]]' is a single command as far as I know
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'm afraid this is issue like "it works in this case, but different approach is better in general" :D Do you want to fix this or should I mark it as wontfix?
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.
No, mark it as false positive instead, it's not that it only works in this case. Codeclimate wants us not to use '$?' variable at all, which cant be done in this case.
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.
Shellcheck (code climate) is fine with approach I suggested as solution. It does not have problem with $?
.
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.
then you found a way around its checks, you still use indirect check ($?) not direct, which is what was reported
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.
- Since the command and its status test are decoupled, inserting an innocent command like
echo "make finished"
aftermake
will cause theif
statement to silently start comparingecho
's status instead.- The value of
$?
is overwritten by[
/[[
, so you can't get the original value in the relevant then/else block (e.g.if mycmd; then echo "Success"; else echo "Failed with $?"; fi
).
https://github.com/koalaman/shellcheck/wiki/SC2181
Closes #39 |
Pending review, @lsolodkova |
Code Climate has analyzed commit a95de36 and detected 0 issues on this pull request. View more on Code Climate. |
simple fuzzer using radamsa