Deprecate BSTR String constructor and setValue method

[dbwiddis]

Fixes #1292

As noted in the issue, this is already tested for in c.s.j.p.ByReferencePlatformToStringTest. I used the gc-based test code noted in the issue (and a few hundred instantiations in an array) to reproduce the issue at will before the fix and confirm this fixes that problem. I also added null checks.

This does not protect against all cases, just the two most obvious ones (not throwing exceptions on null and tracking memory when a BSTR constructor is used).

An alternative solution would be to deprecate the BSTR constructor and replace it with a Pointer constructor, making much more clear to the user what is being stored. I think I prefer this (deprecate) alternative but wanted to at least see what it would take to fix the existing one and get feedback.

[matthiasblaesing]

Either this null check is unnessary, or there is a potential problem in line 175.

[dbwiddis]

If bstr is null we never check line 175 and jump straight here. I believe this is the normal case when new BSTRByReference() is instantiated before any value is set (e.g., before setValue() is called)

[matthiasblaesing]

The null check could succeed and after that bstr is updated from a parallel thread and set to null. I suggest to copy the value into a local variable, read that and update bstr if necessary.

[dbwiddis]

True but this is starting to get to be too much work for a band-aid fix to a poor API to start with. I'm really leaning more towards a simple deprecation.

[dbwiddis]

Thanks for the review, but the more I try to think about all the possible ways this approach needs to be protected against itself the more in favor I am of a simple and effective:

1. Create a new pointer constructor.
2. Deprecate the BSTR constructor, delegating to the pointer constructor using getPointer() which is essentially what's actually happening anyway, we'd just expose it to the API and document that this class doesn't track the original object.

[dbwiddis]

I pushed my alternate deprecation proposal which preserves the existing implementation and simply makes it more obvious to the user what's happening. If you prefer my earlier iteration, with more safeguards for thread safety I can go that route.

[matthiasblaesing]

I had a further thought about this: In my mind BSTRByReference is not the actual problem. That class class is save, if the BSTR contract is followed in the first place. From my perspective the real problems are:

• com.sun.jna.platform.win32.WTypes.BSTR.BSTR(String) (the String based contructor)
• com.sun.jna.platform.win32.WTypes.BSTR.setValue(String) (the function that pretents, that you can modify a BSTR

It would be possible to implement both functions with the Ole functions, but it would be a leaky abstraction and deprecating both functions sounds like the sanest function.

[matthiasblaesing]

I would not change BSTRByReference - the definition of that class is perfectly safe if people don't shoot themselves in the foot first. That definition only begins to break apart, if people works explicitly against the spec.

[dbwiddis]

I would not change BSTRByReference

I still think it needs some explicit javadoc reference even if it's not deprecated. And there is a new BSTR(String) call internally that needs to be removed if we're deprecating that. Let me see if there's a better way to do that.

[matthiasblaesing]

I would not change BSTRByReference

I still think it needs some explicit javadoc reference even if it's not deprecated.

You mean you want to spell out in BSTRByReference, that the user is responsible to correctly allocate the BSTR? Ok, sounds sane.

And there is a new BSTR(String) call internally that needs to be removed if we're deprecating that. Let me see if there's a better way to do that.

Where do you see that in BSTRByReference?

jna/contrib/platform/src/com/sun/jna/platform/win32/WTypes.java

Lines 152 to 173 in 81a18de

	public class BSTRByReference extends ByReference {
	public BSTRByReference() {
	super(Native.POINTER_SIZE);
	}
	public BSTRByReference(BSTR value) {
	this();
	setValue(value);
	}
	public void setValue(BSTR value) {
	this.getPointer().setPointer(0, value.getPointer());
	}
	public BSTR getValue() {
	return new BSTR(getPointer().getPointer(0));
	}
	public String getString() {
	return this.getValue().getValue();
	}
	}

I only see a new BSTR(Pointer) call (167).

[dbwiddis]

Sorry, I misremembered. It's been a long week. You're right... I reverted to the original, added javadocs and a null check on the String output. Should be good to go I think.

[matthiasblaesing]

Looks good to me - I suggest to adjust the change log entry and get this in. Thank you.