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

ManagedBuffer::shift overwrites the entire buffer with zero on a large shift value #156

Closed
c272 opened this issue Oct 4, 2022 · 2 comments · Fixed by #157
Closed

ManagedBuffer::shift overwrites the entire buffer with zero on a large shift value #156

c272 opened this issue Oct 4, 2022 · 2 comments · Fixed by #157
Assignees

Comments

@c272
Copy link
Contributor

c272 commented Oct 4, 2022

If given a shift value larger than or equal to the length of the buffer region being shifted, ManagedBuffer will simply fill the entire buffer with zero, rather than the intended buffer region:

if (offset <= -len || offset >= len) {
fill(0);
return;
}

This can be reproduced with the following MRE:

#include <MicroBit.h>
MicroBit uBit;

int main() {
    uBit.init();
    ManagedBuffer buf(32);

    //Fill some unrelated very important bytes with data.
    buf.fill(0xAA, 0, sizeof(int));

    //Fill some other region, and then attempt a large shift.
    buf.fill(0xBB, 16, 16);
    buf.shift(1000, 16, 16);

    //Our very important data has now been overwritten with zero...
    int* important_data = reinterpret_cast<int*>(buf.getBytes());
    while (true)
        uBit.display.scroll(*important_data);
}

This seems like a simple fix, and seems to work fine when L306-309 are changed to instead read:

if (offset <= -len || offset >= len) {
        fill(0, start, len);
        return;
}
@finneyj
Copy link
Contributor

finneyj commented Oct 4, 2022

Many thanks @c272!

That's a good catch. I agree the current behaviour seems rather counter intuitive, and your suggested fix looks good.

Would you like to work up a PR and contribute?

@c272
Copy link
Contributor Author

c272 commented Oct 4, 2022

No problem, I've submitted a PR at #157. Please let me know if anything needs changing!

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

Successfully merging a pull request may close this issue.

3 participants