Re: [Passwordsafe-devel] SecString class

[Edward E. <pas...@ed...>]

--- James Curran <Ja...@No...> wrote:
>   I've just join in, and figure that I'd throw in my two cent on
> the recent conversations:

That's what we're here for :).  Thanks for taking time to contribute.
I haven't heard from Jim in months, I don't think he's actively
working on the project anymore.  Not that I've done anything either...

There are more reasons not to subclass std::string than just (lack of)
virtual destructors, but first let me respond to a few of your
comments.


>   Actually, there is NO problem here, because there is no
> conceivable situation where the dtor would not be called.  To prove
> this, you just have to look at the one (inconceivable) scenario,
> where it is a problem:

It appears such a scenario is conceivable, given that you conceived of
one :).  Seriously, I admit it's not gonna be a common occurence, but
the fact is it *can* happen.  Secure software has to consider every
possibility, not just the likely scenarios.


> void Func()
> {
>   std::string* pStr = GetANewString();
>   // use pStr
>
>   delete pStr;   // ~std::string() called, not subclass dtor
> }

That is one way it could happen.  Also consider smart pointers.
It's common to use smart pointers for holding objects in STL
containers to reduce copying overhead.  Say you have a
set< smart_ptr<std::string> >, you could wrap a smart_ptr around
a SecString and place it in the container too.  Only now the
smart_ptr will call the std::string destructor on the SecString.

I agree that with sufficient attentiveness you can avoid doing these
things.  But why require the extra effort when you can just as easily
ensure it can *never* happen by not subclassing std::string?


>   Further, we must have the ability (and desire) to change
> GetANewString() (to return a subclass of std::string), but (and
> here's
> the key), we must be UNABLE to change Func().  However, considering
> that
> code, could there be any possible reason for writing that idiom,
> *unless* it was to use pStr polymorphically?  In which case, the
> programmer, *expects* a subclass of string there, and so the code is
> broken before you touched a thing.
>
> In other words, we'd be making design decisions based on the
> theoretically possibility of breaking code that's already broken.

There are plenty of reasons besides polymorphic behavior to use
pointers: reducing object copying, improving locality of reference,
adding another level of indirection.  So no, the use of a string*
does not automatically imply that we are dealing with subclasses
of string.

Additionally, unless we're writing a compiler, an operating system,
and all the libraries from scratch, there will be code which we don't
control.  If your Func above resides in one of those places, we
wouldn't be able to change its code.  There could well be a Func
that takes a std::string* as an argument.

However, even if you're above argument were true, I don't think it's
good justification for subclassing std::string.  Programmers forget
things.  Programmers make changes without fully understanding the
code.  Programmers make mistakes.  I do these things frequently, and
I consider myself an above-average programmer.

By not subclassing std::string, we prevent anyone from ever committing
one of these mistakes.  Many more programmers besides you and I will
be working on the project code in its lifetime.  The reliability and
maintainability of the code should be primary concerns.


>   The problem with the alternative you suggest of create a new
> instantiation of basic_string with a specific allocator, to that you
> lose the IS-A relationship of inheritance.  For example, an existing
> function which took a std::string& parameter would work perfectly
> with a
> string-derived class, but not with a new basic_string type, since it
> would have not relationship with std::string.

Exactly, that's the point.  A SecString is *not* a std::string.  It
contains data which the program must control extremely carefullly.
You don't want a SecString being passed to any function that handles
std::strings.  Almost all functions handle their data very carelessly
(from a security standpoint): making copies, passing it around,
outputting the contents to logs or consoles.  You don't want any of
these things happening to a SecString.

Compatibility shouldn't be an issue anyway.  There should be very few
places which need access to SecString's data.  Look at PasswordSafe.
Say a SecString holds a password.  How many places need to set the
string data?  Two - one when the user first inputs the password,
another when decrypting it in from the password storage file.  How
many places need to read the data?  Two - one for displaying the
password on screen, another to encrypt the password for storage on
disk.  I have a hard time thinking of any library functions you
would want to pass a password to that only accept std::strings.

As an aside, there's no reason functions which handle std::string's
can't be compatible with a SecString instantiated from basic_string.
If instead of this
    void foo(std::string& s)
you write this
    template <class _Char, class _Alloc>
    void foo(basic_string<_Char, _Alloc>& s)
then foo will accept any instantiations of basic_string, including
std::string.

Now forget eveything I've just said.  Suppose there are no problems
with the lack of a virtual destructor.  I can still think of at least
two crucial reasons not to subclass std::string.

The first problem is slicing.  If SecString is a subclass of
std::string, and you pass SecString *by value* to a function which
accepts a std::string, that function will get a copy of SecString's
data in a std::string object.  Now all the protections built into
SecString go right out the window, because the function isn't using
a SecString object anymore.

Similarly, you have problems of assignment between string classes.
If I have string s and SecString t, and SecString subclasses string,
then I can assign t to s: s = t.  Again we have the same problem as
before: SecString's data residing in a std::string object.  The data
has no more protection than if we used std::string in the first
place.

All in all, I think an allocator is the safest way to go.  That still
lets us use all the STL algorithms on SecString, which should be more
than sufficient for PasswordSafe's needs.

Regards,
Edward Elliott