Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Struct.NumberField.getString() downcasts to int #94

Closed
ahilananantha opened this issue Dec 3, 2016 · 4 comments
Closed

Struct.NumberField.getString() downcasts to int #94

ahilananantha opened this issue Dec 3, 2016 · 4 comments
Milestone

Comments

@ahilananantha
Copy link

If somebody does a String.valueOf(myStruct.foo)... where foo is a u_int64_t field... instead of String.valueOf(myStruct.foo.get()) which will be simple and safe, it looks to me like the code is doing an unintentional downcast to int.

I'm seeing the issue in Struct.NumberField:

    /**
     * Returns a string representation of this <code>Address</code>.
     *
     * @return a string representation of this <code>Address</code>.
     */
    @Override
    public java.lang.String toString() {
        return java.lang.Integer.toString(intValue(), 10);
    }

the intValue() call will be to Struct.IntegerAlias.intValue():

    @Override
    public int intValue() {
        return (int) get();
    }

which does the downcast. Perhaps call longValue() instead?

@headius
Copy link
Member

headius commented Dec 12, 2016

It does appear that this logic does not support 64-bit values at all. Changing the toString is only one small part of it; note that NumberField.longValue also calls intValue and upcasts it to a long. It's unclear to me if we can just switch all of these classes to actually treat the contained value as a long.

Thoughts?

@ahilananantha
Copy link
Author

ahilananantha commented Dec 12, 2016

AFAICT, NumberField.longValue is always overridden in the places that matter, e.g. IntegerAlias, Signed64, Unsigned64, SignedLong, UnsignedLong. Perhaps you could make all those methods abstract to avoid confusion?

@ahilananantha
Copy link
Author

And calling longValue() will still be wrong if the number is a float. Looks like it's more correct to make the method abstract.

@ahilananantha
Copy link
Author

The shortest fix seems to be to copy over the toString() method of Signed64 / Unsigned64 / SignedLong / UnsignedLong into IntegerAlias and Unsigned32. The delicate series of overrides of longValue() and toString() in derived classes seems to handle every other case.

Alternately we can make longValue() and toString() abstract so that the code is easier to read but lengthier.

ahilananantha pushed a commit to ahilananantha/jnr-ffi that referenced this issue Jan 4, 2017
Since NumberField.toString() downcasts to int, it needs to be overridden
for number fields bigger than int. This was handled in other places
but was missed for Unsigned32 and IntegerAlias.
headius added a commit that referenced this issue Jan 25, 2017
Override toString() in Unsigned32 and IntegerAlias to fix #94.
@headius headius modified the milestone: 2.1.3 Jan 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants