SharkCagey / SharkCage

1 stars 3 forks source link

Type mismatch in smart pointers #103

Closed bencikpeter closed 6 years ago

bencikpeter commented 6 years ago

While implementing #102 I was digging around in SecuritySetup.cpp and I believe that all smart pointers in that file need to be rewriten. Explanation:

this is what they look like: std::unique_ptr<PSID, decltype(local_free_deleter<PSID>)> sid((PSID*)::LocalAlloc(LPTR, cb_sid), local_free_deleter<PSID>);

this is what they are supposed to replace: PSID sid = (PSID) new BYTE[cb_sid];

but what they actually mean is this: PSID* sid = (PSID*) new BYTE[cb_sid];

How do I know? Later they are used in calls that expect PSID and not PSID* such as LookupAccountName.

Why does it work even though it´s wrong? I´ve been digging around in the definitions and found this: typedef PVOID PSID; and typedef void *PVOID;

So they are all basically void pointers and it seems to be, that implicit conversion between void* and void** combined with the fact that data that it refers to are the same despite the fact they are wrong type ensures that the program works.

However, it manifests in lines like this:

PSID group_sid_raw = group_sid.get();
explicit_access_group.Trustee.ptstrName = static_cast<LPWSTR>(group_sid_raw);

where impicit conversions void** -> void* and void* -> LPWSTR are allright but there is no implicit conversion of void** -> LPWSTR and that´s why this line would not work even though it´s supposed to:

explicit_access_group.Trustee.ptstrName = static_cast<LPWSTR>(group_sid.get());

@SailReal @DonatJR Do you think I am right in my reasoning or am I missing something critical?

DonatJR commented 6 years ago

I was thinking about this before, but for some reason I couldn't get it to work with SID instead of PSID (got it running on the first try right now, though) and then went on to work on something else.

I think you are right and we should fix this. Do you prefer to use SID as the pointer type or void (counterintuitively void would be more correct as PSID is also a void-pointer, like you pointed out before)?

bencikpeter commented 6 years ago

I´d say using SID is more self-explenatory than void ... even hthough that would introduce some type-casting to every use right? Maybe some custom define such as #define void custom_sid would eliminate both... neccessary type-casting of SID and confusion of void

What do you say?

DonatJR commented 6 years ago

Yes, using SID would indeed introduce type-casting. Will use these definitions:

typedef void Sid;
typedef std::unique_ptr<Sid, decltype(local_free_deleter<Sid>)> SidPointer;