RfidResearchGroup / proxmark3

Iceman Fork - Proxmark3
http://www.icedev.se
GNU General Public License v3.0
3.86k stars 1.02k forks source link

Flaky test nonce2key #2501

Closed elboulangero closed 6 days ago

elboulangero commented 1 week ago

Hello,

I just updated the proxmark3 package that we distribute in Kali Linux to latest version v4.18994, and I observe that a test is flaky, it fails every now and then:

staticnested_2x1nt_rf08s test           [ OK ] ✔️ 
staticnested_2x1nt_rf08s test           [ OK ] ✔️ 
lf EM4x70 recover test 2/3              [ OK ] ✔️ 
lf EM4x70 recover test 3/3              [ OK ] ✔️ 
staticnested_2x1nt_rf08s_1key test      rm: cannot remove 'keys_5c467f63_00_456ace4e.dic': No such file or directory
rm: cannot remove 'keys_5c467f63_00_e56f9fa2.dic': No such file or directory
[ OK ] ✔️ 
staticnested_2x1nt_rf08s_1key test      Warning: Cannot open keys_5c467f63_00_e56f9fa2_filtered.dic
rm: cannot remove 'keys_5c467f63_00_456ace4e_filtered.dic': No such file or directory
rm: cannot remove 'keys_5c467f63_00_e56f9fa2_filtered.dic': No such file or directory
[ OK ] ✔️ 

Testing nonce2key: ./tools/mfc/card_only/nonce2key
nonce2key exists                        [ OK ] ✔️
nonce2key test                          [ FAIL ] ❌ 
Execution trace:

------------------------------------------------------------

Tests [ FAIL ] ❌

make[1]: *** [Makefile:117: mfc_card_only/check] Error 1
make[1]: *** Waiting for unfinished jobs....

[...]

make[1]: Leaving directory '/<<PKGBUILDDIR>>'
    rm -fr -- /tmp/dh-xdg-rundir-wZOF9W3g
dh_auto_test: error: make -j8 check returned exit code 2

Note the command that we run the tests with parallelization enabled (make -j8), so maybe you won't see the issue if you don't run tests the same way.

Best,

iceman1001 commented 1 week ago

You are running make check in parallel? This will trigger problems.

make clean;
make -j check

The make has to complete before you can run the tests.

make clean;
make -j;
make check;
elboulangero commented 1 week ago

The make has to complete before you can run the tests

Of course! To clarify, we build with make all, and the we test with make -j check. Full log for a build at https://gitlab.com/kalilinux/packages/proxmark3/-/jobs/7801033487/viewer for example.

I'm pretty sure that it's a regression in latest version. Looking at our pipelines at https://gitlab.com/kalilinux/packages/proxmark3/-/pipelines, there has been a fair number of successful builds before, and the test started to be flaky only after I started to build v4.18994.

I tried a run without parallel (cf. https://gitlab.com/arnaudr/proxmark3/-/jobs/7815347554/viewer), and I noticed that:

So we end up with two TESTALL=true checks running in parallel, and that's why it's flaky. I have the impression that tools/pm3_tests.sh needs to be updated to match how it's invoked from the top-level Makefile.

iceman1001 commented 6 days ago

Aha, This is a great explanation. Looking at bit in pm3_tests.sh and top-level Makefile there are support but not correctly used since they now consists of two checks both.

Card_only tests

TESTSTATICNESTED=false
TESTNONCE2KEY=false

Card_reader tests

TESTMFKEY=false
TESTMFNONCEBRUTE=false

I wonder if we can use CHECKARGS to send in the correct params...

iceman1001 commented 6 days ago

Would you mind testing this to see if it solves the issue for you? I tried locally and it works for me.

mfc_card_only/check: FORCE
    CHECKARGS = "nonce2key staticnested"
    $(info [*] CHECK $(patsubst %/check,%,$@))
    $(Q)$(BASH) tools/pm3_tests.sh $(CHECKARGS) $(patsubst %/check,%,$@)
mfc_card_reader/check: FORCE
    CHECKARGS = "mfkey mf_nonce_brute"
    $(info [*] CHECK $(patsubst %/check,%,$@))
    $(Q)$(BASH) tools/pm3_tests.sh $(CHECKARGS) $(patsubst %/check,%,$@)
elboulangero commented 6 days ago

Doesn't work for me

CHECKARGS = "nonce2key staticnested"
[*] CHECK mfc_card_reader
/bin/sh: 1: CHECKARGS: not found
make[1]: *** [Makefile:116: mfc_card_only/check] Error 127
make[1]: *** Waiting for unfinished jobs....

I'm trying again with:

mfc_card_only/check: FORCE
    $(info [*] CHECK $(patsubst %/check,%,$@))
    $(Q)$(BASH) tools/pm3_tests.sh $(CHECKARGS) nonce2key staticnested
mfc_card_reader/check: FORCE
    $(info [*] CHECK $(patsubst %/check,%,$@))
    $(Q)$(BASH) tools/pm3_tests.sh $(CHECKARGS) mfkey mf_nonce_brute

In the top-level Makefile, CHECKARGS is documented as being something the caller can use, so better not use it and leave it for the caller

iceman1001 commented 6 days ago

Did you pull latest ?
I see the following failed message in your log and there was a fix pushed this morning.

mkversion create test env: 'git': No such file or directory

running your idea with direct args, works on my machine (wsl / ubuntu).

elboulangero commented 6 days ago

Did you pull latest ?

Ah no, sorry, I just applied the changes in Makefile as you showed in the snippet above. I applied it on version 4.18994.

Direct args as I mentioned in https://github.com/RfidResearchGroup/proxmark3/issues/2501#issuecomment-2348359316 also works for me

I can open a MR if you want, but otherwise feel free to apply it yourself (to simplify) if you think it's the right change

iceman1001 commented 6 days ago

Added your suggested fix.

elboulangero commented 6 days ago

Thanks!