Hi Wei. Checking in to see if any of these changes made it into crypto++. I was hoping to get these issues fixed so everyone using crypto++ can be error free under valgrind without having to add suppression files for each one of the reported issues.
Chris On Thu, Feb 7, 2008 at 6:30 PM, Chris Morgan <[EMAIL PROTECTED]> wrote: > Hi Wei. > > Any update on this patch and the other valgrind issue? Do you need > anything else from me at this point? > > Chris > > > > On Jan 19, 2008 2:17 PM, Chris Morgan <[EMAIL PROTECTED]> wrote: >> Hey Wei. Thanks for fixing those issues. >> >> I've upgraded from either 5.5.0 or 5.5.1 to 5.5.2 and valgrind finds a >> couple more issues. >> >> ... >> Testing SymmetricCipher algorithm Salsa20. >> ......==21004== >> ==21004== Conditional jump or move depends on uninitialised value(s) >> ==21004== at 0x48D125: TestSymmetricCipher(std::map<std::string, >> std::string, std::less<std::string>, >> std::allocator<std::pair<std::string const, std::string> > >&) >> (char_traits.h:254) >> ==21004== by 0x48F832: TestDataFile(std::string const&, unsigned&, >> unsigned&) (datatest.cpp:517) >> ==21004== by 0x48FCA8: RunTestDataFile(char const*) (datatest.cpp:559) >> ==21004== by 0x45A6AC: ValidateSalsa() (validat1.cpp:1339) >> ==21004== by 0x461DF4: ValidateAll(bool) (validat1.cpp:93) >> ==21004== by 0x44A064: Validate(int, bool, char const*) (test.cpp:781) >> ==21004== by 0x45190C: main (test.cpp:298) >> ==21004== >> ==21004== Conditional jump or move depends on uninitialised value(s) >> ==21004== at 0x48D144: TestSymmetricCipher(std::map<std::string, >> std::string, std::less<std::string>, >> std::allocator<std::pair<std::string const, std::string> > >&) >> (datatest.cpp:321) >> ==21004== by 0x48F832: TestDataFile(std::string const&, unsigned&, >> unsigned&) (datatest.cpp:517) >> ==21004== by 0x48FCA8: RunTestDataFile(char const*) (datatest.cpp:559) >> ==21004== by 0x45A6AC: ValidateSalsa() (validat1.cpp:1339) >> ==21004== by 0x461DF4: ValidateAll(bool) (validat1.cpp:93) >> ==21004== by 0x44A064: Validate(int, bool, char const*) (test.cpp:781) >> ==21004== by 0x45190C: main (test.cpp:298) >> ==21004== >> ==21004== Conditional jump or move depends on uninitialised value(s) >> ==21004== at 0x48D41B: TestSymmetricCipher(std::map<std::string, >> std::string, std::less<std::string>, >> std::allocator<std::pair<std::string const, std::string> > >&) >> (char_traits.h:254) >> ==21004== by 0x48F832: TestDataFile(std::string const&, unsigned&, >> unsigned&) (datatest.cpp:517) >> ==21004== by 0x48FCA8: RunTestDataFile(char const*) (datatest.cpp:559) >> ==21004== by 0x45A6AC: ValidateSalsa() (validat1.cpp:1339) >> ==21004== by 0x461DF4: ValidateAll(bool) (validat1.cpp:93) >> ==21004== by 0x44A064: Validate(int, bool, char const*) (test.cpp:781) >> ==21004== by 0x45190C: main (test.cpp:298) >> . >> Tests complete. Total tests = 7. Failed tests = 0. >> >> Sosemanuk validation suite running... >> >> Testing SymmetricCipher algorithm Sosemanuk. >> ==21004== >> ==21004== Conditional jump or move depends on uninitialised value(s) >> ==21004== at 0x48D125: TestSymmetricCipher(std::map<std::string, >> std::string, std::less<std::string>, >> std::allocator<std::pair<std::string const, std::string> > >&) >> (char_traits.h:254) >> ==21004== by 0x48F832: TestDataFile(std::string const&, unsigned&, >> unsigned&) (datatest.cpp:517) >> ==21004== by 0x48FCA8: RunTestDataFile(char const*) (datatest.cpp:559) >> ==21004== by 0x45A67C: ValidateSosemanuk() (validat1.cpp:1345) >> ==21004== by 0x461E01: ValidateAll(bool) (validat1.cpp:94) >> ==21004== by 0x44A064: Validate(int, bool, char const*) (test.cpp:781) >> ==21004== by 0x45190C: main (test.cpp:298) >> ==21004== >> ==21004== Conditional jump or move depends on uninitialised value(s) >> ==21004== at 0x48D41B: TestSymmetricCipher(std::map<std::string, >> std::string, std::less<std::string>, >> std::allocator<std::pair<std::string const, std::string> > >&) >> (char_traits.h:254) >> ==21004== by 0x48F832: TestDataFile(std::string const&, unsigned&, >> unsigned&) (datatest.cpp:517) >> ==21004== by 0x48FCA8: RunTestDataFile(char const*) (datatest.cpp:559) >> ==21004== by 0x45A67C: ValidateSosemanuk() (validat1.cpp:1345) >> ==21004== by 0x461E01: ValidateAll(bool) (validat1.cpp:94) >> ==21004== by 0x44A064: Validate(int, bool, char const*) (test.cpp:781) >> ==21004== by 0x45190C: main (test.cpp:298) >> .==21004== >> ==21004== Conditional jump or move depends on uninitialised value(s) >> ==21004== at 0x48D144: TestSymmetricCipher(std::map<std::string, >> std::string, std::less<std::string>, >> std::allocator<std::pair<std::string const, std::string> > >&) >> (datatest.cpp:321) >> ==21004== by 0x48F832: TestDataFile(std::string const&, unsigned&, >> unsigned&) (datatest.cpp:517) >> ==21004== by 0x48FCA8: RunTestDataFile(char const*) (datatest.cpp:559) >> ==21004== by 0x45A67C: ValidateSosemanuk() (validat1.cpp:1345) >> ==21004== by 0x461E01: ValidateAll(bool) (validat1.cpp:94) >> ==21004== by 0x44A064: Validate(int, bool, char const*) (test.cpp:781) >> ==21004== by 0x45190C: main (test.cpp:298) >> ==21004== >> ==21004== Conditional jump or move depends on uninitialised value(s) >> ==21004== at 0x48D41D: TestSymmetricCipher(std::map<std::string, >> std::string, std::less<std::string>, >> std::allocator<std::pair<std::string const, std::string> > >&) >> (char_traits.h:254) >> ==21004== by 0x48F832: TestDataFile(std::string const&, unsigned&, >> unsigned&) (datatest.cpp:517) >> ==21004== by 0x48FCA8: RunTestDataFile(char const*) (datatest.cpp:559) >> ==21004== by 0x45A67C: ValidateSosemanuk() (validat1.cpp:1345) >> ==21004== by 0x461E01: ValidateAll(bool) (validat1.cpp:94) >> ==21004== by 0x44A064: Validate(int, bool, char const*) (test.cpp:781) >> ==21004== by 0x45190C: main (test.cpp:298) >> . >> Tests complete. Total tests = 2. Failed tests = 0. >> >> VMAC validation suite running... >> ... >> >> >> This appears to indicate that datatest.cpp:321 has a comparison with >> an uninitialized value. I can't see where encrypted, ciphertext, >> xorDigest or ciphertextXorDigest are initialized but it appears that >> one of those, or test, is not initialized. I was having a hard time >> tracking this one down because I don't understand the code well and >> hoping you or someone else familiar with the code could quickly spot >> it. >> >> >> I've attached another patch for zdeflate.cpp. Apparently another >> version of the constructor is being called with 5.5.2 which brought >> out the same initialization issue with m_deflateLevel, but with >> another variant of the constructor. >> >> Chris >> >> >> >> >> >> On Aug 11, 2007 8:27 PM, Wei Dai <[EMAIL PROTECTED]> wrote: >> > Sorry for the late response. I've confirmed these issues, but want to fix >> > them a bit differently. Chris, please try the attached patch and let me >> > know >> > if it fixes the valgrind problems. >> > >> > >> > ----- Original Message ----- >> > From: "Chris Morgan" <[EMAIL PROTECTED]> >> > To: "Crypto++ Users" <[EMAIL PROTECTED]>; "Wei Dai" >> > <[EMAIL PROTECTED]> >> > Sent: Tuesday, August 07, 2007 6:26 PM >> > Subject: Re: [PATCH] - fix some issues identified by valgrind >> > >> > >> > > >> > > Any news on these patches? >> > > >> > > Chris >> > > >> > > >> > > >> > > On 8/5/07, Chris Morgan <[EMAIL PROTECTED]> wrote: >> > >> Detected these issues while running the cryptest.exe validation under >> > >> valgrind. >> > >> >> > >> The zdeflate.cpp change was because SetDeflateLevel() was using >> > >> m_deflateLevel before m_deflateLevel was initialized. I'm not sure >> > >> this is the way you would want to fix the issue since there are a >> > >> couple of different way to correct this. >> > >> >> > >> >> > >> words.h code was resulting in an issue because we were calling >> > >> memcpy() with the same source and destination, something the man page >> > >> for memcpy() says not to do. Here is the output from valgrind. It >> > >> seems as if crypto++ could optimize this situation away higher up in >> > >> the architecture but I wasn't sure. >> > >> >> > >> ==13599== Source and destination overlap in memcpy(0x4068098, 0x4068098, >> > >> 16) >> > >> ==13599== at 0x4C237F3: memcpy (mc_replace_strmem.c:116) >> > >> ==13599== by 0x535756: CryptoPP::PositiveAdd(CryptoPP::Integer&, >> > >> CryptoPP::Integer const&, CryptoPP::Integer const&) (words.h:24) >> > >> ==13599== by 0x535A64: >> > >> CryptoPP::Integer::operator+=(CryptoPP::Integer const&) >> > >> (integer.cpp:3559) >> > >> ==13599== by 0x535FA2: CryptoPP::Integer >> > >> CryptoPP::StringToInteger<char>(char const*) (integer.cpp:3014) >> > >> ==13599== by 0x53611E: CryptoPP::Integer::Integer(char const*) >> > >> (integer.cpp:3027) >> > >> ==13599== by 0x472054: ValidateBBS() (validat2.cpp:55) >> > >> ==13599== by 0x461DFC: ValidateAll(bool) (validat1.cpp:97) >> > >> ==13599== by 0x44A2A6: Validate(int, bool, char const*) >> > >> (test.cpp:781) >> > >> ==13599== by 0x451C57: main (test.cpp:298) >> > >> passed 49ea2cfdb01064a0bbb92af101dac18a94f7b7ce >> > >> passed 2af101dac18a94f7b7ce >> > >> >> > >> >> > >> Similar change in filters.cpp as in words.h, again, here is the >> > >> valgrind output in case it helps figure out why the higher level code >> > >> is calling the lower level code with the same source and destination. >> > >> >> > >> passed cryptosystem key validation >> > >> ==13097== Source and destination overlap in memcpy(0x5ED05A0, 0x5ED05A0, >> > >> 20) >> > >> ==13097== at 0x4C237F3: memcpy (mc_replace_strmem.c:116) >> > >> ==13097== by 0x501BF7: CryptoPP::ArraySink::Put2(unsigned char >> > >> const*, unsigned long, int, bool) (filters.cpp:512) >> > >> ==13097== by 0x501368: CryptoPP::Filter::Output(int, unsigned char >> > >> const*, unsigned long, int, bool, std::string const&) >> > >> (filters.cpp:115) >> > >> ==13097== by 0x503073: CryptoPP::HashFilter::Put2(unsigned char >> > >> const*, unsigned long, int, bool) (filters.cpp:730) >> > >> ==13097== by 0x55B7B9: >> > >> CryptoPP::P1363_MGF1KDF2_Common(CryptoPP::HashTransformation&, >> > >> unsigned char*, unsigned long, unsigned char const*, unsigned long, >> > >> unsigned char const*, unsigned long, bool, unsigned) (cryptlib.h:763) >> > >> ==13097== by 0x4391AE: >> > >> CryptoPP::DL_KeyDerivationAlgorithm_P1363<CryptoPP::Integer, true, >> > >> CryptoPP::P1363_KDF2<CryptoPP::SHA1> >> > >> >::Derive(CryptoPP::DL_GroupParameters<CryptoPP::Integer> const&, >> > >> unsigned char*, unsigned long, CryptoPP::Integer const&, >> > >> CryptoPP::Integer const&, CryptoPP::NameValuePairs const&) const >> > >> (pubkey.h:506) >> > >> ==13097== by 0x4325BF: >> > >> CryptoPP::DL_EncryptorBase<CryptoPP::Integer>::Encrypt(CryptoPP::RandomNumberGenerator&, >> > >> unsigned char const*, unsigned long, unsigned char*, >> > >> CryptoPP::NameValuePairs const&) const (pubkey.h:1237) >> > >> ==13097== by 0x471324: >> > >> CryptoSystemValidate(CryptoPP::PK_Decryptor&, CryptoPP::PK_Encryptor&, >> > >> bool) (validat2.cpp:169) >> > >> ==13097== by 0x474794: ValidateDLIES() (validat2.cpp:399) >> > >> ==13097== by 0x461E40: ValidateAll(bool) (validat1.cpp:102) >> > >> ==13097== by 0x44A2A6: Validate(int, bool, char const*) >> > >> (test.cpp:781) >> > >> ==13097== by 0x451C57: main (test.cpp:298) >> > >> passed encryption and decryption >> > >> >> > >> >> > >> With these three changes the valgrind output here looks good. No more >> > >> errors and no leaks, btw I'm using the --leak-check=full option as >> > >> well. >> > >> >> > >> Chris >> > >> >> > >> >> > > >> > > >> > > >> > >> > --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the "Crypto++ Users" Google Group. To unsubscribe, send an email to [EMAIL PROTECTED] More information about Crypto++ and this group is available at http://www.cryptopp.com. -~----------~----~----~----~------~----~------~--~---
