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

sys/base64: Fix, unit test cleanup, and benchmark #14400

Merged
merged 4 commits into from Jul 2, 2020

Conversation

maribu
Copy link
Member

@maribu maribu commented Jun 30, 2020

Contribution description

  • Cherry-picked @mjurczak fix for the decode buffer size estimation (which could result in a buffer overflow of up to one byte note being catched)
  • Made using the API less pain by accepting void pointers for buffers (non-breaking change, as unsigned char * is implicitly casted to void *)
  • Cleaned up the unit test to fix basic code quality issued
  • Added a unit test to check the base64_estimate_{de,en}code_size()
  • Added a benchmark for base64 (this will come in useful to review a follow up PR)

Testing procedure

The unit tests should no detect an issue in base64_estimate_{de,en}code_size()

Issues/PRs references

None

@maribu maribu added Area: tests Area: tests and testing framework Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: sys Area: System labels Jun 30, 2020
@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Jun 30, 2020
@maribu maribu added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 1, 2020
@maribu
Copy link
Member Author

maribu commented Jul 1, 2020

Somehow the build was previously not queued, toggling CI: ready for build seems to have solved the issue.

@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 1, 2020
@benpicco
Copy link
Contributor

benpicco commented Jul 1, 2020

Looks like the unit test will now fail

base64_tests.test_base64_13_size_estimation (tests/unittests/tests-base64/tests-base64.c 495) exp 3 was 0

@maribu maribu added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 2, 2020
@maribu
Copy link
Member Author

maribu commented Jul 2, 2020

No need to run tests on hardware. The compilation tests include a run of the unit tests on the native board.

@maribu
Copy link
Member Author

maribu commented Jul 2, 2020

@mjurczak: I cherry-picked your suggested fix and included it in the PR. This way, the credit for and authorship of the commits stays with you.

@maribu maribu changed the title sys/base64: Add test and cleanup sys/base64: Fix, unit test cleanup, and benchmark Jul 2, 2020
@maribu
Copy link
Member Author

maribu commented Jul 2, 2020

With @mjurczak fix included, the unit tests are now passing. I updated the PR title and the description accordingly.

@maribu maribu removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 2, 2020
@maribu
Copy link
Member Author

maribu commented Jul 2, 2020

Fixed some style issues, while we're at it.

(I will let Murdock run again when I have squashed.)

@fjmolinas fjmolinas added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Jul 2, 2020
Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the benchmark have an automatic test?

It might be good to split the benchmark from this PR, that way this PR can limit itself to cleanups and the bugfix and can get in right away. But if you add the 01_test.pyscript right away we can get everything in at once, I don't mind.

@maribu
Copy link
Member Author

maribu commented Jul 2, 2020

Should the benchmark have an automatic test?

I'm not sure what an automatic test should do. A single benchmark run will provide a raw number, that lacks context. (Or two raw numbers, one for encode and one for decode.) IMO, at least a second benchmark result from the same hardware is needed to have any meaningful information.

What we could do (if we want to run automatic tests), is to feed a database with the raw results to see how performance changes over time. But maybe such considerations should be done independently form this PR and applied later on to all benchmarks.

@fjmolinas
Copy link
Contributor

I'm not sure what an automatic test should do. A single benchmark run will provide a raw number, that lacks context. (Or two raw numbers, one for encode and one for decode.) IMO, at least a second benchmark result from the same hardware is needed to have any meaningful information.

What we could do (if we want to run automatic tests), is to feed a database with the raw results to see how performance changes over time. But maybe such considerations should be done independently form this PR and applied later on to all benchmarks.

I was just thinking on the most basic check, what we do for other benches tests/bench_xtimer/tests/01-run.py, just a simple test that says "the application works". My comment comes mainly from the impression that we now ask for a 01-run.py for new applications.

@maribu
Copy link
Member Author

maribu commented Jul 2, 2020

I was just thinking on the most basic check, what we do for other benches tests/bench_xtimer/tests/01-run.py, just a simple test that says "the application works".

Ah, OK! I added the test.

@fjmolinas
Copy link
Contributor

Please squash @maribu and re-trigger ci.

mjurczak and others added 2 commits July 2, 2020 11:10
Fixed required result buffer size underestimation in base64_estimate_decode_size() function.
This is a non-breaking change, as `unsigned char *` can implicitly be converted
to `void *`.
@maribu maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 2, 2020
- Added unit test for `base64_estimate_{de,en}code_size()`
- Mark constant stuff as `const`
- Use `memcmp()` for comparing memory
- Do not use variable size arrays
- Various code style issue
@maribu
Copy link
Member Author

maribu commented Jul 2, 2020

I fixed a typo in the test input detected by the static tests (and the corresponding base64) and added the missing empty line flake8 complained about. I squashed right away.

@miri64 miri64 added this to the Release 2020.07 milestone Jul 2, 2020
Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK, bug pointed out by @mjurczak is addressed with a unit-test exposing it. Cleanups go according yo our coding style. Lets see what murdock has to say.

@miri64
Copy link
Member

miri64 commented Jul 2, 2020

Murdock likes it too.

@miri64 miri64 merged commit 339e3fa into RIOT-OS:master Jul 2, 2020
2 checks passed
@maribu maribu deleted the base64_tests branch July 2, 2020 13:12
@maribu
Copy link
Member Author

maribu commented Jul 2, 2020

@mjurczak: Thank you very much for reporting and fixing the issue!

Thanks everyone for the reviews.

@mjurczak
Copy link
Contributor

mjurczak commented Jul 3, 2020

@maribu I'm stunned with the fast reaction. Great job everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants