Copyable handles are probably unsound about winsafe HOT 8 CLOSED

Making the handles non-Copy wouldn't fix this issue either. An user could close a handle and still keep it indefinitely, then call a function with this handle later – pontentially causing the behavior you described. That's a rather hard problem to solve.

One solution I can think of is to make the function calls operate by reference (instead of by value), and "closing" functions would then set the handle to null. Subsequent calls would then return an ERROR_INVALID_HANDLE (if anything), which could be properly caught as ordinary errors.

What do you think?

from winsafe.

What would be most intuitive to me is that the handles are non-Copy, non-Clone, do not have an explicit close function, and automatically close on drop. Is there a reason that wouldn’t work?

The iterator used for enumerating values would have to borrow the handle, I assume. That seems sensible to me, to ensure the handle outlives the iterator.

from winsafe.

This would be the perfect solution – in fact, my very first implementation tried to follow this concept. The problem is that sometimes you should not close the handles.

From the top of my head:

• basically all HWND functions, like CreateWindowEx and GetParent;

• the messy GDI objects, whose handles are sometimes returned again by SelectObject;

• the HIMAGELIST, which is created and, if passed to a ListView with LVS_SHAREIMAGELISTS style, should not be deleted (the ListView takes ownership of it).

After facing many weird situations, I eventually gave up the idea.

However...

Rethinking this solution, I could simply segregate the handles types. Some of them would be returned wrapped in to a smart pointer implementing Deref, and they would implement Drop to call the close functions, thus removing the manual close altogether. This would work like a glove for cases like HKEY and HFILE.

The problem with this approach is that I would have two "categories" of handles, which would be potentially confusing.

Another idea is having high-level structs, like File does over HFILE, to properly work on the resource cleanup. In this specific case, a high-level Registry struct.

from winsafe.

Hm, yes, there is some complexity there. Some handles are pretty simple, but others not so much.

For the HGDIOBJ case, I thought of two ideas.

Option 1

SelectObject takes an HGDIOBJ by value and std::mem::forgets it. It then takes the return value of the raw function call, i.e. the previously selected value, wraps it in a new handle wrapper, and returns it. This is pretty easy to understand, but it is somewhat limited: you can’t keep an HGDIOBJ in a read-only place (e.g. a struct to which you have only a shared reference) and still be able to select it into a DC, and it also means you can’t select the same object into two DCs at the same time.

Option 2

Instead of SelectObject, you could do a scope-bound thing. So the DC type would have something like a with_selected method, which would be used something like this:

let my_brush = …;
let my_dc = …;
// At this time, the default brush is selected.
my_dc.with_selected(&my_brush, || {
  // Inside this lambda, my_brush is selected.
  // It is taken by shared reference, so it can still be reused elsewhere, but it cannot be destroyed.
});
// When the lambda completes, the original default brush is re-selected to the DC before `with_selected` returns.
// `my_brush` has now been unlocked and is safe to destroy.

I’ve seen this kind of pattern elsewhere, though I’m not sure how ergonomic it would be in this case. It also forces an extra SelectObject call to restore the original brush; I’m not sure whether that will have a performance impact or not, and I’m not that familiar with GDI so I’m not actually even sure whether you might need to do that anyway before destroying the DC.

from winsafe.

Option 1 has the limitations you point out.

Option 2 has the problem to be completely alien to the Win32 API – if creating a whole new layer is the answer, then I'd rather implement a whole new struct over HGDIOBJ, leaving it alone.

The idea of setting the handle to null in "closing" functions still seems to be the most attractive to me, so far. It would prevent accidental handle reuse, which is the original problem you pointed out, and you are 100% right about this.

from winsafe.

That’s fair. Although, for a HANDLE, what null value would you use? NULL or INVALID_HANDLE_VALUE?

from winsafe.

Actually, would that even help? You’d still have to make the handles non-Copy, non-Clone, otherwise one handle would be nulled out, but all the others wouldn’t. And even then, AFAICT it wouldn’t stop someone from deleting a GDI object that was currently selected, would it? Is it actually possible to do this safely, or do the handles themselves need to be unsafe and then the higher-level wrappers be safe?

from winsafe.

I believe INVALID_HANDLE_VALUE would be clearer for the given intent.

Yes, the handles would still have to be non-Copy and non-Clone.

It has been a long time since I wrote my last GDI program, but as far as I remember, deleting a GDI object which is in use simply will revert the resource to its stock one, not causing any other harms. I'll have to test this. The high-level wrapper will certainly prevent this, though.

from winsafe.