Improves security AND makes the code more readable, I like this.

ACK.

Using ::c_cstr at API boundaries does have the drawback that '\0' characters are not allowed -- the canonical form of converting between string types is to use data() and size(), or .begin()/.end(). Then again, I don't think you can enter those in form fields anyway :)

Though it does make some code a bit easier to read, it is no more secure (in fact, with the additional conversions, one could argue its less secure, though not really). Anyway, meh.

Less secure? How?

.get_str().c_str() instead of .get_str() in several places means the string (likely) gets copied twice in memory instead of once.

I really don't see how this could make things less secure.

• By passing all the passphrases as SecureString all the way into the CWallet API, this makes sure the passphrases only exist in mlocked memory (except in Qt and inside the JSON parser, but that was already the case before, and in the case of Qt there is little to do against it). Before, they were passed in plain std::strings.
• As I understand the conversions don't cause any extra allocation/deallocation of insecure memory, the conversions just pass memory around (and there's not that many more) from one mlocked area to another, which is filled with zeros before freeing.
• By using a specific SecureString type it is signaled clearly that the data inside it should be handled with care.

Sigh, seems that sometimes you just want to disagree with things for the sake of disagreeing... do you really like arguing that much?

No, it does not copy anything (it can't, where would it copy to?). c_str() simply returns a pointer to the data inside the string.

re: 1 yes, but those strings were previously mlocked, so no security is gained.

re: 2 yes, that is my bad, I had just forgotten that those were returning pointers, not creating new objects...just my C++ inexperience showing...

Let my rephrase my original comment:
Makes code easier to read, but doesnt add security, probably worth pulling. Meh.

1. In the case of bitcoin-qt they weren't. That was my reason for saying this is more secure.

2. STL has a bad name from the MSVC 6.0 implementation, but luckily is not that inefficient :)

Also this makes it harder to forget manually mlocking/munlocking (or maybe worse, skipping munlocking in the case of an exception so that memory stays locked indefinitely) in future usages of the API.