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

Find at least "5" (there are more) raw memory leaks due to missing or mis-handled implementation #65

Closed
anonimal opened this Issue Jan 10, 2016 · 5 comments

Comments

Projects
None yet
3 participants
@anonimal
Collaborator

anonimal commented Jan 10, 2016

Easter-egg hunt in January?

21:15:06       zzz | for your todo list: <orignal> there are approcimately 5 more places sending raw memory
21:48:55     str4d | <orignal> they are all marked with TODO: <-- raw memory locations (I assume he means padding)

Note: focus on any TODO's, especially those marked with ???

Running tally (If there are more, they will be resolved by #58):

9dd3dc0
1dcbe27
#123
#125
#165

Note: regarding the time-frame to complete d649e8a in #165:

  1. I was waiting for #140 to be resolved but, in our meetings, we eventually agreed to not wait any longer and pursue a patch instead
  2. I wanted to focus on finishing #149 before flipping branches and committing d649e8a. This could have easily gone the other way but, since we are not in production, and the leak is actually not raw (it is initialized, just not with random padding as java i2p specifies), I chose to focus on the bigger work involved in #149 and, once completed, resolve this ticket.

@anonimal anonimal self-assigned this Jan 10, 2016

@KanediasTheMaker

This comment has been minimized.

KanediasTheMaker commented Jan 15, 2016

What prevents from using valgrind?

@anonimal

This comment has been minimized.

Collaborator

anonimal commented Jan 15, 2016

Nothing. Running valgrind at this stage is comical. Please, see for yourself.

This ticket is an extension of a recent IRC discussion and is related to raw memory contents leaking into message space because of improper implementation and not necessarily improperly deallocated "memory leaks" in the traditional sense (though, that is also an agenda item (see #58)).

See 9dd3dc0 as an example.

If you'd like to learn more, visit us on IRC (see CONTRIBUTING.md)

@KanediasTheMaker

This comment has been minimized.

KanediasTheMaker commented Jan 15, 2016

not necessarily improperly deallocated "memory leaks" in the traditional sense

Ahh, got it

If you'd like to learn more, visit us on IRC

Sorry, I'm currently involved in other FLOSS project, but I'll keep an eye on your fork that seems to be far less toxic than orig[i]nal one.

For now the only thing I can help with is armv7h GNU/Linux compile and day-to-day use

@anonimal

This comment has been minimized.

Collaborator

anonimal commented Jan 16, 2016

@KanediasTheMaker that's great! Any ARM contributions would be much appreciated (though we don't officially support ARM yet). For now, if anyone happens to post ARM-related bugs, you have the honor of being a first-responder 😄

JFTR: the original C++ implementation predates i2pd (as said in the README.md)

@majestrate

This comment has been minimized.

Contributor

majestrate commented Jan 16, 2016

for the record, i am about to push some revisions that fixes 1 of them,
which happens to be in SSUSession, I suspect more in are in either SSU or
NTCP, most likely NTCP.

On Sat, Jan 16, 2016 at 12:13 PM, anonimal notifications@github.com wrote:

@KanediasTheMaker https://github.com/KanediasTheMaker that's great! Any
ARM contributions would be much appreciated (though we don't officially
support ARM yet). For now, if anyone happens to post ARM-related bugs,
you have the honor of being a first-responder [image: 😄]

JFTR: the original http://git.repo.i2p.xyz/w/i2pcpp.git C++
implementation predates i2pd (as said in the README.md
https://github.com/monero-project/kovri/blob/master/README.md)


Reply to this email directly or view it on GitHub
#65 (comment)
.

@anonimal anonimal changed the title from Find the elusive (and mysterious) "5" raw memory leaks to Find at least "5" (there are more) raw memory leaks due to missing or mis-handled implementation Feb 7, 2016

anonimal added a commit to anonimal/kovri that referenced this issue Feb 8, 2016

anonimal added a commit that referenced this issue Feb 8, 2016

Merge pull request #123
456a14d NTCPSession: fix Phase2 raw memory leak. Refs #65. (anonimal)
eb6d2e6 NTCPSession: refactor datatypes + identifiers. (anonimal)
5d64e62 NTCPSession: resolve a refactor TODO. (anonimal)

anonimal added a commit to anonimal/kovri that referenced this issue Feb 10, 2016

I2NP: fix tunnel build request raw memory leak.
* Added spec documentation
* Referencing monero-project#65

anonimal added a commit that referenced this issue Feb 14, 2016

Merge pull request #133
de59b56 Config: implement single-switch opts, refactor. (anonimal)
06cd79b Reseed: update server list + certificates. (anonimal)
e6e8b15 Refactor: resolve all using-directive TODO's. (anonimal)
5153a94 I2NP/NetDb: move NETWORK_ID to Version.h (anonimal)
037e4f6 I2NP: fix tunnel build request raw memory leak. (anonimal)
456a14d NTCPSession: fix Phase2 raw memory leak. Refs #65. (anonimal)
eb6d2e6 NTCPSession: refactor datatypes + identifiers. (anonimal)
5d64e62 NTCPSession: resolve a refactor TODO. (anonimal)

anonimal added a commit that referenced this issue Feb 17, 2016

Merge pull request #136
51dcb23 License/Docs: implement 0MQ 22/C4.1 + revise docs. (anonimal)
5dc950f License/Docs: minor revisions to license. (anonimal)
fd859a7 Crypto: update RandInRange() backend + unit-test. (anonimal)
9c330cb Crypto: create true RandInRange(). Fixes #130. (anonimal)
de59b56 Config: implement single-switch opts, refactor. (anonimal)
06cd79b Reseed: update server list + certificates. (anonimal)
e6e8b15 Refactor: resolve all using-directive TODO's. (anonimal)
5153a94 I2NP/NetDb: move NETWORK_ID to Version.h (anonimal)
037e4f6 I2NP: fix tunnel build request raw memory leak. (anonimal)
456a14d NTCPSession: fix Phase2 raw memory leak. Refs #65. (anonimal)
eb6d2e6 NTCPSession: refactor datatypes + identifiers. (anonimal)
5d64e62 NTCPSession: resolve a refactor TODO. (anonimal)
8999af5 Reseed: improve non-standard ports patch (581dd9e) (anonimal)
581dd9e Reseed: fix bug for servers with non-standard ports. (anonimal)
8eb187f CMake: update OSX OpenSSL. (anonimal)
0958052 Minor ammendments to reflect new lib organization. (anonimal)
cf48ea0 Fix build, forgot to include crypto/Rand.h (EinMByte)
a5de2d6 Fix bitmask used to select SSU fragment size (thanks orignal). (EinMByte)
1dcbe27 Fill padding with random bytes in NTCP (thanks psi). (EinMByte)
d23138d Add bounds check to SU3 fileNameLength (thanks psi). (EinMByte)
2dc4d74 Fix typo in ElGamal.cpp. (EinMByte)
20859e6 Review PR #112 (resolves #114). (EinMByte)
0bff9a2 Cleanup (following cpplint advice) (EinMByte)
aeca942 Clean up comments in DSA and ElGamal unit tests. (EinMByte)
1e457f3 Rename crypto unit test files for consistency. (EinMByte)
7c5315f add copyright (Jeff Becker)
9e70f59 fix up elg tests (Jeff Becker)
3c0bca4 use main prng in Elgamal Key Generation (Jeff Becker)
d4ffd85 add elg tests and fix up dsa tests (Jeff Becker)
748ec5b fix up elgamal, add bounds check and zero pad check (Jeff Becker)
43f582f add dsa tests (Jeff Becker)
6298dfb make it compile (Jeff Becker)
79620dc reorganize existing crypto unit tests into their own files (Jeff Becker)
5792af6 Fix #109. (EinMByte)
193c3f4 remove OpenSSL link instructions from building.md (Riccardo Spagni)
1f8d3f0 find installed OpenSSL on OS X (Riccardo Spagni)
835cfea Update MacOSX build requirements. Resolves #37. (anonimal)
4bbe89d Resolve #107. (EinMByte)
43f14e1 Move config file constants to Config.h. (EinMByte)
2f39034 Separated libcore, libclient and kovri application (issue #98) (EinMByte)
6efa143 Move Daemon to kovri-main. (EinMByte)
e0f9c73 Merge libclient and libapi. (EinMByte)
d2c0908 Remove coreVersion + stat_uptime. Bump to 0.9.24. (anonimal)
9a14807 Remove redundant #pragma once in Daemon.h. (EinMByte)
76c1cf2 Fix race condition in Transports.cpp. (EinMByte)
9afe4bd Add core/util/Log.h to Daemon.cpp. Fixes #85. (anonimal)
e330dcd don't send ssu packets if they are too big (also covers underflow of size_t which in this case made kovri segfault last night) (Jeff Becker)
24e9b71 fix #85 (Jeff Becker)
53c9867 Huge cleanup of logging implementation. (anonimal)
ab6fae4 Cherry-pick remaining doc/crypto commits from #81 (anonimal)
47223f8 Cherry-pick commits from #81. (anonimal)
661b002 Revert #81 before cherry-picking. (anonimal)
c45f521 fix up i2lua to conform to style guide better (Jeff Becker)
b4f94eb check for dsax >= dsaq (Jeff Becker)
8e77042 (probably) fixed issue with CryptoPP exceptions (Jeff Becker)
e768333 * add documentation strings * add i2p::Buffer for pre filled buffers * various changes to unfux crypto parts (Jeff Becker)
803c799 fix up lua parts, implement i2lua.GetRouterByHash (Jeff Becker)
ede65b2 remove linting related things (Jeff Becker)
88da250 more indentation fixups (Jeff Becker)
14cf9aa more style fixups (Jeff Becker)
3829173 linting (Jeff Becker)
15594e4 Update .codedocs + .gitignore. (anonimal)
b567bba Add .codedocs file and CodeDocs badge to README.md (Paul Novotny)
5889c60 if not logging still start daemon (Jeff Becker)
502d187 fix up PRNG bits (Jeff Becker)
d4e577b add linting helper tools (Jeff Becker)
60173bf Massive refactor of #72. Update style guidelines. (anonimal)
9eae20b we don't need a dummy rng for eddsa anymore (Jeff Becker)
5f473be * use function pointers and NOT std::function in benchmarks as doing so crashes benchmarks occasionally (Jeff Becker)
94ea36f add i2lua documentation (Jeff Becker)
88a511e fix boost::log link error (Jeff Becker)
8605937 * make lua interpreter actually work (Jeff Becker)
243c898 add initial boilerplate for lua interpreter (jeff)
51d9a21 fix benchmarks (jeff)
a8b2b44 fix typo (jeff)
26a4482 compiles and links but untested refactor of all parts using CryptoPP's PRNG to use i2p::crypto::Rand* functions which wrap CryptoPP's PRNG such that it can be swapped out with relative ease if needed. (jeff)
39b513a fix i2pcontrol crash (jeff)
2429f3a add tunnels.cfg reload (jeff)

@anonimal anonimal referenced this issue Mar 28, 2016

Closed

Bump to .25 #161

1 of 1 task complete

@anonimal anonimal closed this in d649e8a Apr 12, 2016

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