Skip to content
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

Get rid of BigBuf_get_addr() #899

Open
slurdge opened this issue Jun 15, 2020 · 7 comments
Open

Get rid of BigBuf_get_addr() #899

slurdge opened this issue Jun 15, 2020 · 7 comments

Comments

@slurdge
Copy link
Contributor

slurdge commented Jun 15, 2020

This is blocking doegox/proxmark-internal#247.
Right now, various functions are using the "address" of BigBuf as a magic variable to pass around data. Clearly this is prone to error and cannot be used with a proper allocator. So we should remove all calls to the BigBuf_get_addr() and replace them with a BigBuf_malloc() or equivalent function.
Especially, we need to find what is the required space by those functions which often declare "I'll take as much as I need and pray that I don't overflow"

@iceman1001
Copy link
Collaborator

O boy, this actually means documenting how stuff works on the device side :)

@slurdge
Copy link
Contributor Author

slurdge commented Jun 16, 2020

This is related to doegox/proxmark-internal#243.
I'm quite sure if we touch it something will break.

@iceman1001
Copy link
Collaborator

... yeah..

putting a blank t55x7 card on top of the antenna and run lf search will trigger much.
I adjusted the stack limit to 9K, it doesn't warn any longer but now clone, read that t55x7 card doesn't work...

a can of worms.

@slurdge
Copy link
Contributor Author

slurdge commented Jun 18, 2020

Yeah, my guesstimate was the stack before was around 12K because we increased bigbuf from 40->48K.
However the commands still should work because we didn't reorder much.
I started looking at the usage of get_addr(), it ain't pretty...

@doegox doegox transferred this issue from another repository Jun 25, 2020
@doegox doegox transferred this issue from another repository Jul 11, 2020
@iceman1001
Copy link
Collaborator

Not that many places left for BigBuf_get_addr() calls. This below is from the FPGA branch, but more and more have been converted into bb_malloc style.

armsrc/start.c
26:    next_free_memory = BigBuf_get_addr();

armsrc/pcf7931.c
19:    uint8_t *dest = BigBuf_get_addr();

armsrc/lfsampling.c
137:            data.buffer = BigBuf_get_addr();
149:        data.buffer = BigBuf_get_addr();
404:    uint8_t *dest = BigBuf_get_addr();
490:    uint8_t *dest = BigBuf_get_addr();
552:    uint8_t *dest = BigBuf_get_addr();

armsrc/lfops.c
505:    signed char *dest = (signed char *)BigBuf_get_addr();
655:    uint32_t *buf = (uint32_t *)BigBuf_get_addr();
709:    char *dest = (char *)BigBuf_get_addr();
812:    uint8_t *buf = BigBuf_get_addr();
901:    uint8_t *dest = BigBuf_get_addr();
1019:    uint8_t *dest = BigBuf_get_addr();
1032:    uint8_t *dest = BigBuf_get_addr();
1045:    uint8_t *dest = BigBuf_get_addr();
1056:    uint8_t *dest = BigBuf_get_addr();
1062:    uint8_t *dest = BigBuf_get_addr();
1132:    uint8_t *dest = BigBuf_get_addr();
1179:    uint8_t *dest = BigBuf_get_addr();
1235:    uint8_t *dest = BigBuf_get_addr();
1345:    uint8_t *dest = BigBuf_get_addr();
1450:    uint8_t *dest = BigBuf_get_addr();
1525:    uint8_t *dest = BigBuf_get_addr();
2061:    uint8_t *buf = BigBuf_get_addr();

armsrc/i2c.c
789:    uint8_t *fwdata = BigBuf_get_addr();

armsrc/felica.c
752:    uint8_t *dest = BigBuf_get_addr();

armsrc/appmain.c
324:    uint8_t *test_data = BigBuf_get_addr();
1508:            uint8_t *mem = BigBuf_get_addr();
1689:            uint8_t *mem = BigBuf_get_addr();
1743:            uint8_t *mem = BigBuf_get_addr();

armsrc/Standalone/lf_icehid.c
81:    uint8_t *dest = BigBuf_get_addr();
133:    uint8_t *dest = BigBuf_get_addr();
193:    uint8_t *dest = BigBuf_get_addr();
237:    uint8_t *dest = BigBuf_get_addr();

armsrc/Standalone/lf_em4100rwc.c
141:    bba = BigBuf_get_addr();

armsrc/Standalone/lf_em4100rswb.c
314:    bba = BigBuf_get_addr();

armsrc/Standalone/lf_em4100emul.c
88:    bba = BigBuf_get_addr();

armsrc/Standalone/hf_14asniff.c
100:        uint8_t *trace_buffer = BigBuf_get_addr();

armsrc/BigBuf.c
170:    uint8_t *trace = BigBuf_get_addr();

@slurdge
Copy link
Contributor Author

slurdge commented Jul 15, 2020

That's awesome!
I'm not too worried about the lf_*.c stuff, from a quick glance, this is more of the same, so replacing one should mean replacing all pretty easily.
The big one IMHO is the sneaky last one: it's related to trace, and I suspect all trace function silently assume this...
However if there is only that one left, then maybe we can do the switch to the new allocator at the same time we are removing it ...

@iceman1001
Copy link
Collaborator

to be honest, I think this bigbuf / stack connected issues was the reason for my problems with Hitag2 stuff.
I think bringing this to the surface gave us a chance to solve it properly once and for all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants