One comment: YES please. Sent from my iPad
> On Jul 31, 2015, at 07:19, Jeffrey Walton <[email protected]> wrote: > > Currently, DEFAUT_CHANNEL and AAD_CHANNEL are strings allocated in > cryptlib.cpp and subject to all the translation unit rules regarding static > initialization. Its been pain point for me in the past, and I think Bizyaev > is experiencing it now. > > I'd like to propose two subtle changes to help manage that initialization > issue without breaking user code. First is a static inline function that > creates a local static to force early initialization: > > //***** cryptlib.h *****// > > //! the default channel for BufferedTransformation, equal to the empty string. > // New code should call the DefaultChannel() function. > extern CRYPTOPP_DLL const std::string DEFAULT_CHANNEL; > inline static const std::string& DefaultChannel() { > static const std::string ch = ""; > return ch; > } > > //! channel for additional authenticated data, equal to the string "AAD". > // New code should call AadChannel() function. > extern CRYPTOPP_DLL const std::string AAD_CHANNEL; > inline static const std::string& AadChannel() { > static const std::string ch = "AAD"; > return ch; > } > > The multiple copies should be folded into one by the optimizer (if multiple > copies are present). > > And second: > > //***** cryptlib.cpp *****// > > const std::string DEFAULT_CHANNEL = DefaultChannel(); > const std::string AAD_CHANNEL = AadChannel(); > > Finally, I'd like to try and get this into 5.6.3 since its one of those > annoying and hard to track down issues. > > A patch for testing is below (and attached). It only modified > cryptlib.{h|cpp}, *test.cpp, and validat?.cpp. > > Any comments or objections? > > Jeff > > ******************** > > $ cat channels.diff > diff --git a/cryptlib.cpp b/cryptlib.cpp > index 43df08b..2ded98c 100644 > --- a/cryptlib.cpp > +++ b/cryptlib.cpp > @@ -32,9 +32,8 @@ CRYPTOPP_COMPILE_ASSERT(sizeof(word64) == 8); > CRYPTOPP_COMPILE_ASSERT(sizeof(dword) == 2*sizeof(word)); > #endif > > -const std::string DEFAULT_CHANNEL; > -const std::string AAD_CHANNEL = "AAD"; > -const std::string &BufferedTransformation::NULL_CHANNEL = DEFAULT_CHANNEL; > +const std::string DEFAULT_CHANNEL = DefaultChannel(); > +const std::string AAD_CHANNEL = AadChannel(); > > class NullNameValuePairs : public NameValuePairs > { > diff --git a/cryptlib.h b/cryptlib.h > index 207b1e6..dd52511 100644 > --- a/cryptlib.h > +++ b/cryptlib.h > @@ -768,11 +768,21 @@ public: > bool Wait(unsigned long milliseconds, CallStack const& callStack); > }; > > -//! the default channel for BufferedTransformation, equal to the empty > std::string > +//! the default channel for BufferedTransformation, equal to the empty > string. > +// New code should call the DefaultChannel() function. > extern CRYPTOPP_DLL const std::string DEFAULT_CHANNEL; > +inline static const std::string& DefaultChannel() { > + static const std::string ch = ""; > + return ch; > +} > > -//! channel for additional authenticated data, equal to "AAD" > +//! channel for additional authenticated data, equal to the string "AAD". > +// New code should call AadChannel() function. > extern CRYPTOPP_DLL const std::string AAD_CHANNEL; > +inline static const std::string& AadChannel() { > + static const std::string ch = "AAD"; > + return ch; > +} > > //! interface for buffered transformations > > @@ -803,8 +813,6 @@ extern CRYPTOPP_DLL const std::string AAD_CHANNEL; > class CRYPTOPP_DLL CRYPTOPP_NO_VTABLE BufferedTransformation : public > Algorithm, public Waitable > { > public: > - // placed up here for CW8 > - static const std::string &NULL_CHANNEL; // same as DEFAULT_CHANNEL, > for backwards compatibility > > BufferedTransformation() : Algorithm(false) {} > > @@ -929,18 +937,18 @@ public: > size_t PeekWord32(word32 &value, ByteOrder > order=BIG_ENDIAN_ORDER) const; > > //! move transferMax bytes of the buffered output to target > as input > - lword TransferTo(BufferedTransformation &target, lword > transferMax=LWORD_MAX, const std::string &channel=DEFAULT_CHANNEL) > + lword TransferTo(BufferedTransformation &target, lword > transferMax=LWORD_MAX, const std::string &channel=DefaultChannel()) > {TransferTo2(target, transferMax, channel); return > transferMax;} > > //! discard skipMax bytes from the output buffer > virtual lword Skip(lword skipMax=LWORD_MAX); > > //! copy copyMax bytes of the buffered output to target as > input > - lword CopyTo(BufferedTransformation &target, lword > copyMax=LWORD_MAX, const std::string &channel=DEFAULT_CHANNEL) const > + lword CopyTo(BufferedTransformation &target, lword > copyMax=LWORD_MAX, const std::string &channel=DefaultChannel()) const > {return CopyRangeTo(target, 0, copyMax, channel);} > > //! copy copyMax bytes of the buffered output, starting at > position (relative to current position), to target as input > - lword CopyRangeTo(BufferedTransformation &target, lword > position, lword copyMax=LWORD_MAX, const std::string > &channel=DEFAULT_CHANNEL) const > + lword CopyRangeTo(BufferedTransformation &target, lword > position, lword copyMax=LWORD_MAX, const std::string > &channel=DefaultChannel()) const > {lword i = position; CopyRangeTo2(target, i, > i+copyMax, channel); return i-position;} > > #ifdef CRYPTOPP_MAINTAIN_BACKWARDS_COMPATIBILITY > @@ -965,18 +973,18 @@ public: > //! skip count number of messages > virtual unsigned int SkipMessages(unsigned int > count=UINT_MAX); > //! > - unsigned int TransferMessagesTo(BufferedTransformation > &target, unsigned int count=UINT_MAX, const std::string > &channel=DEFAULT_CHANNEL) > + unsigned int TransferMessagesTo(BufferedTransformation > &target, unsigned int count=UINT_MAX, const std::string > &channel=DefaultChannel()) > {TransferMessagesTo2(target, count, channel); return > count;} > //! > - unsigned int CopyMessagesTo(BufferedTransformation &target, > unsigned int count=UINT_MAX, const std::string &channel=DEFAULT_CHANNEL) > const; > + unsigned int CopyMessagesTo(BufferedTransformation &target, > unsigned int count=UINT_MAX, const std::string &channel=DefaultChannel()) > const; > > //! > virtual void SkipAll(); > //! > - void TransferAllTo(BufferedTransformation &target, const > std::string &channel=DEFAULT_CHANNEL) > + void TransferAllTo(BufferedTransformation &target, const > std::string &channel=DefaultChannel()) > {TransferAllTo2(target, channel);} > //! > - void CopyAllTo(BufferedTransformation &target, const > std::string &channel=DEFAULT_CHANNEL) const; > + void CopyAllTo(BufferedTransformation &target, const > std::string &channel=DefaultChannel()) const; > > virtual bool GetNextMessageSeries() {return false;} > virtual unsigned int NumberOfMessagesInThisSeries() const > {return NumberOfMessages();} > @@ -986,13 +994,13 @@ public: > //! \name NON-BLOCKING TRANSFER OF OUTPUT > //@{ > //! upon return, byteCount contains number of bytes that have > finished being transfered, and returns the number of bytes left in the > current transfer block > - virtual size_t TransferTo2(BufferedTransformation &target, > lword &byteCount, const std::string &channel=DEFAULT_CHANNEL, bool > blocking=true) =0; > + virtual size_t TransferTo2(BufferedTransformation &target, > lword &byteCount, const std::string &channel=DefaultChannel(), bool > blocking=true) =0; > //! upon return, begin contains the start position of data > yet to be finished copying, and returns the number of bytes left in the > current transfer block > - virtual size_t CopyRangeTo2(BufferedTransformation &target, > lword &begin, lword end=LWORD_MAX, const std::string > &channel=DEFAULT_CHANNEL, bool blocking=true) const =0; > + virtual size_t CopyRangeTo2(BufferedTransformation &target, > lword &begin, lword end=LWORD_MAX, const std::string > &channel=DefaultChannel(), bool blocking=true) const =0; > //! upon return, messageCount contains number of messages > that have finished being transfered, and returns the number of bytes left in > the current transfer block > - size_t TransferMessagesTo2(BufferedTransformation &target, > unsigned int &messageCount, const std::string &channel=DEFAULT_CHANNEL, bool > blocking=true); > + size_t TransferMessagesTo2(BufferedTransformation &target, > unsigned int &messageCount, const std::string &channel=DefaultChannel(), bool > blocking=true); > //! returns the number of bytes left in the current transfer > block > - size_t TransferAllTo2(BufferedTransformation &target, const > std::string &channel=DEFAULT_CHANNEL, bool blocking=true); > + size_t TransferAllTo2(BufferedTransformation &target, const > std::string &channel=DefaultChannel(), bool blocking=true); > //@} > > //! \name CHANNELS > diff --git a/datatest.cpp b/datatest.cpp > index d0f69c6..5595c4a 100644 > --- a/datatest.cpp > +++ b/datatest.cpp > @@ -61,7 +61,7 @@ const std::string & GetRequiredDatum(const TestData &data, > const char *name) > return i->second; > } > > -void RandomizedTransfer(BufferedTransformation &source, > BufferedTransformation &target, bool finish, const std::string > &channel=DEFAULT_CHANNEL) > +void RandomizedTransfer(BufferedTransformation &source, > BufferedTransformation &target, bool finish, const std::string > &channel=DefaultChannel()) > { > while (source.MaxRetrievable() > (finish ? 0 : 4096)) > { > @@ -492,16 +492,16 @@ void TestAuthenticatedSymmetricCipher(TestData &v, > const NameValuePairs &overrid > > if (macAtBegin) > RandomizedTransfer(sm, df, true); > - sh.CopyTo(df, LWORD_MAX, AAD_CHANNEL); > + sh.CopyTo(df, LWORD_MAX, AadChannel()); > RandomizedTransfer(sc, df, true); > - sf.CopyTo(df, LWORD_MAX, AAD_CHANNEL); > + sf.CopyTo(df, LWORD_MAX, AadChannel()); > if (!macAtBegin) > RandomizedTransfer(sm, df, true); > df.MessageEnd(); > > - RandomizedTransfer(sh, ef, true, AAD_CHANNEL); > + RandomizedTransfer(sh, ef, true, AadChannel()); > RandomizedTransfer(sp, ef, true); > - RandomizedTransfer(sf, ef, true, AAD_CHANNEL); > + RandomizedTransfer(sf, ef, true, AadChannel()); > ef.MessageEnd(); > > if (test == "Encrypt" && encrypted != ciphertext+mac) > diff --git a/gfpcrypt.h b/gfpcrypt.h > index 0590254..1537b31 100644 > --- a/gfpcrypt.h > +++ b/gfpcrypt.h > @@ -454,7 +454,7 @@ public: > if (DHAES_MODE) > { > byte L[8] = {0,0,0,0}; > - PutWord(false, BIG_ENDIAN_ORDER, L+4, > word32(encodingParameters.size())); > + PutWord(false, BIG_ENDIAN_ORDER, L, word64(8 * > encodingParameters.size())); > mac.Update(L, 8); > } > mac.Final(ciphertext + plaintextLength); > @@ -483,7 +483,7 @@ public: > if (DHAES_MODE) > { > byte L[8] = {0,0,0,0}; > - PutWord(false, BIG_ENDIAN_ORDER, L+4, > word32(encodingParameters.size())); > + PutWord(false, BIG_ENDIAN_ORDER, L, word64(8 * > encodingParameters.size())); > mac.Update(L, 8); > } > if (!mac.Verify(ciphertext + plaintextLength)) > diff --git a/test.cpp b/test.cpp > index b357119..7ad18d3 100644 > --- a/test.cpp > +++ b/test.cpp > @@ -621,7 +621,7 @@ void SecretShareFile(int threshold, int nShares, const > char *filename, const cha > > channel = WordToString<word32>(i); > fileSinks[i]->Put((byte *)channel.data(), 4); > - channelSwitch->AddRoute(channel, *fileSinks[i], > DEFAULT_CHANNEL); > + channelSwitch->AddRoute(channel, *fileSinks[i], > DefaultChannel()); > } > > source.PumpAll(); > @@ -671,7 +671,7 @@ void InformationDisperseFile(int threshold, int nShares, > const char *filename) > > channel = WordToString<word32>(i); > fileSinks[i]->Put((byte *)channel.data(), 4); > - channelSwitch->AddRoute(channel, *fileSinks[i], > DEFAULT_CHANNEL); > + channelSwitch->AddRoute(channel, *fileSinks[i], > DefaultChannel()); > } > > source.PumpAll(); > > > > -- > -- > 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. > --- > You received this message because you are subscribed to the Google Groups > "Crypto++ Users" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to [email protected]. > For more options, visit https://groups.google.com/d/optout. > <channels.diff> -- -- 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. --- You received this message because you are subscribed to the Google Groups "Crypto++ Users" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
