Skip to content

Deprecate BSTR String constructor and setValue method#1300

Merged
dbwiddis merged 1 commit intojava-native-access:masterfrom
dbwiddis:bstr
Jan 29, 2021
Merged

Deprecate BSTR String constructor and setValue method#1300
dbwiddis merged 1 commit intojava-native-access:masterfrom
dbwiddis:bstr

Conversation

@dbwiddis
Copy link
Copy Markdown
Contributor

@dbwiddis dbwiddis commented Jan 19, 2021 •
edited
Loading

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.

@dbwiddis dbwiddis marked this pull request as draft January 19, 2021 21:33
matthiasblaesing
matthiasblaesing reviewed Jan 21, 2021
View reviewed changes
Comment thread contrib/platform/src/com/sun/jna/platform/win32/WTypes.java Outdated
}
// Null check the pointer
Pointer p = getPointer().getPointer(0);
return p != null ? new BSTR(p) : null;
Copy link
Copy Markdown
Member

@matthiasblaesing matthiasblaesing Jan 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@dbwiddis dbwiddis Jan 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Comment thread contrib/platform/src/com/sun/jna/platform/win32/WTypes.java Outdated
return new BSTR(getPointer().getPointer(0));
// If memory address hasn't changed, return the stored BSTR
if (bstr != null) {
if (getPointer().getPointer(0).equals(bstr.getPointer())) {
Copy link
Copy Markdown
Member

@matthiasblaesing matthiasblaesing Jan 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@dbwiddis dbwiddis Jan 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Copy Markdown
Contributor Author

dbwiddis commented Jan 28, 2021

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 dbwiddis marked this pull request as ready for review January 28, 2021 05:14
@dbwiddis
Copy link
Copy Markdown
Contributor Author

dbwiddis commented Jan 28, 2021

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
Copy link
Copy Markdown
Member

matthiasblaesing commented Jan 28, 2021

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
Copy link
Copy Markdown
Member

matthiasblaesing commented Jan 29, 2021

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
Copy link
Copy Markdown
Contributor Author

dbwiddis commented Jan 29, 2021

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
Copy link
Copy Markdown
Member

matthiasblaesing commented Jan 29, 2021

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
Copy link
Copy Markdown
Contributor Author

dbwiddis commented Jan 29, 2021

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
matthiasblaesing approved these changes Jan 29, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@dbwiddis dbwiddis changed the title Prevent losing track of BSTRByReference memory Deprecate BSTR String constructors Jan 29, 2021
@dbwiddis dbwiddis changed the title Deprecate BSTR String constructors Deprecate BSTR String constructor and setValue method Jan 29, 2021
@dbwiddis dbwiddis merged commit 91f139e into java-native-access:master Jan 29, 2021
@dbwiddis dbwiddis deleted the bstr branch January 29, 2021 21:58
@dbwiddis dbwiddis mentioned this pull request Jan 30, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@matthiasblaesing matthiasblaesing matthiasblaesing approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

BSTRByReference constructor loses reference to allocated memory

2 participants

@dbwiddis @matthiasblaesing