This repository has been archived by the owner on Sep 1, 2022. It is now read-only.
ByteStream security fix + new additions, SSU fix + refactoring #810
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
They aren't polluting the namespace yet.
We're no longer using scoped enumerations in SSU.
Allows OutputByteStream to behave more like a sequence container without the overhead *or* functionality of one. Note that, ultimately, using containers will produce better-managed code (see the TODOs).
"Skipped" bytes were not properly initialized when writing, so zero-initialize. We also don't need access to the advancing impl.
and rewrite null-stream test-case.
This was referenced Feb 16, 2018
|
I've re-implemented how we handle our internal buffer as well as several interfaces (not yet pushed to this branch, so you'll still see the failed tests above). Doing so fixes the tests on OSX 10.12 (the only OSX machine I have access to because of monero-project/meta#179), should resolve all OSX failed tests (and possibly OpenBSD), and should also resolve the ARMv7 tests segfault (since it fixes the segfault on my aforementioned ubuntu machine). I'll get the new work pushed sometime within ~24 hours or by Monday. |
Length is decremented / counter incremented when advancing pointer.
|
All tests + live tests pass. Ready to merge. |
anonimal
added a commit
that referenced
this pull request
Feb 20, 2018
6f4e0c7 ByteStream: calculate size from length plus counter (anonimal) b3e3c68 ByteStream: add base input interface + internal vector buffer (anonimal) 9fe1653 SSUSession: fix invalid writing of null padding (anonimal) 8e6989a ByteStream: add asserts + more exception handling (anonimal) cd34ff1 CMake: fix incorrect setting of C release flags (anonimal) b215ba6 ByteStream: add new byte skipping interfaces/impls (anonimal) 23871a1 ByteStream: add buffer-less ctor for container-like stream + test-case (anonimal) 92c1d51 ByteStream: rename advancing function as appropriate (anonimal) 7928c3e SSU: remove all calls to get enum underlying type (anonimal) 77e3a1d SSU: remove packet/data scoped enumerations (anonimal)
This was referenced Mar 2, 2018
Closed
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
By submitting this pull-request, I confirm the following:
Known issues to be resolved before merging:
1. Ubuntu (gcc 5.4.0-6) unit-tests pass but SIGSEGV because of a cryptopp issue that I will soon open. Cryptopp apparently doesn't like how we construct our internal-buffer for the "stream". I cannot reproduce this issue on Arch (gcc 7.3.0 / clang 5.0.1) but will open an issue upstream regardless.Edit: Buildbot cannot reproduce so I can assume for now that the issue is specific to my personal build machine.Edit: this appears to affect our ARMv7 machine https://build.getmonero.org/builders/kovri-all-ubuntu-arm7/builds/613/steps/test/logs/stdio.Apple LLVM version 8.0.0 (clang-800.0.42.1). I don't know how recently that machine has been updated though since many of our build machines are going out of date, I will open an issue for this too. Edit: opened Buildbot: complete backend software upgrades meta#179.In all, I'm becoming very irritated with the bytestream implementation / concept as a whole. While #140 has had the best of intentions, the bytestream is building on - and revolving around - previous spaghetti-code pointer concepts rather then a usable, modular, OOP design. The current bytestream design also prevents us from managing memory in a more guaranteed and interoperable way. This issue too must be opened in the issue tracker. Edit: opened #811.