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

Correctly handle unsigned int values returned from TCP_INFO #7011

Closed
wants to merge 1 commit into
base: 4.1
from

Conversation

@normanmaurer
Member

normanmaurer commented Jul 22, 2017

Motivation:

We used an int[] to store all values that are returned in the struct for TCP_INFO which is not good enough as it uses usigned int values.

Modifications:

  • Change int[] to long[] and correctly cast values.

Result:

No more truncated values.

@normanmaurer normanmaurer requested a review from Scottmitch Jul 22, 2017

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer
Member

normanmaurer commented Jul 22, 2017

@carl-mastrangelo PTAL as well.

@carl-mastrangelo

LGTM.

Side note: there are a few more tcp info fields in later kernels. Not sure which one Netty targets.

@johnou

This comment has been minimized.

Show comment
Hide comment
@johnou

johnou Jul 22, 2017

Contributor

@normanmaurer looks like tests failed.

https://garage.netty.io/teamcity/viewLog.html?buildId=22521&buildTypeId=pulls_netty&guest=1

io.netty.channel.epoll (1)
EpollSocketChannelTest.testTcpInfo

First failure: 7011 #5052 Changes (337) 22 Jul 17 14:37
java.lang.AssertionError
at io.netty.channel.epoll.EpollSocketChannelTest.assertTcpInfo0(EpollSocketChannelTest.java:74)
at io.netty.channel.epoll.EpollSocketChannelTest.testTcpInfo(EpollSocketChannelTest.java:39)

Contributor

johnou commented Jul 22, 2017

@normanmaurer looks like tests failed.

https://garage.netty.io/teamcity/viewLog.html?buildId=22521&buildTypeId=pulls_netty&guest=1

io.netty.channel.epoll (1)
EpollSocketChannelTest.testTcpInfo

First failure: 7011 #5052 Changes (337) 22 Jul 17 14:37
java.lang.AssertionError
at io.netty.channel.epoll.EpollSocketChannelTest.assertTcpInfo0(EpollSocketChannelTest.java:74)
at io.netty.channel.epoll.EpollSocketChannelTest.testTcpInfo(EpollSocketChannelTest.java:39)

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Jul 22, 2017

Member
Member

normanmaurer commented Jul 22, 2017

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Jul 23, 2017

Member

@johnou doh!... my fault. Should be fixed now.

Member

normanmaurer commented Jul 23, 2017

@johnou doh!... my fault. Should be fixed now.

public int state() {
return info[0] & 0xFF;
return (int) info[0];

This comment has been minimized.

@johnou

johnou Jul 23, 2017

Contributor

What about introducing a new int safe cast utility method?

From openjdk 8..

/**
     * Returns the value of the {@code long} argument;
     * throwing an exception if the value overflows an {@code int}.
     *
     * @param value the long value
     * @return the argument as an int
     * @throws ArithmeticException if the {@code argument} overflows an int
     * @since 1.8
     */
    public static int toIntExact(long value) {
        if ((int)value != value) {
            throw new ArithmeticException("integer overflow");
        }
        return (int)value;
    }
@johnou

johnou Jul 23, 2017

Contributor

What about introducing a new int safe cast utility method?

From openjdk 8..

/**
     * Returns the value of the {@code long} argument;
     * throwing an exception if the value overflows an {@code int}.
     *
     * @param value the long value
     * @return the argument as an int
     * @throws ArithmeticException if the {@code argument} overflows an int
     * @since 1.8
     */
    public static int toIntExact(long value) {
        if ((int)value != value) {
            throw new ArithmeticException("integer overflow");
        }
        return (int)value;
    }

This comment has been minimized.

@normanmaurer

normanmaurer Jul 24, 2017

Member

I think we not need this for now.

@normanmaurer

normanmaurer Jul 24, 2017

Member

I think we not need this for now.

@@ -154,7 +154,7 @@ public static LinuxSocket newSocketDomain() {
private static native int getTcpUserTimeout(int fd) throws IOException;
private static native int isIpFreeBind(int fd) throws IOException;
private static native int isIpTransparent(int fd) throws IOException;
private static native void getTcpInfo(int fd, int[] array) throws IOException;
private static native void getTcpInfo(int fd, long[] array) throws IOException;

This comment has been minimized.

@netkins

netkins Jul 23, 2017

MINOR Consider using varargs for methods or constructors which take an array the last parameter. rule

@netkins

netkins Jul 23, 2017

MINOR Consider using varargs for methods or constructors which take an array the last parameter. rule

This comment has been minimized.

@normanmaurer

normanmaurer Jul 24, 2017

Member

thats fine...

@normanmaurer

normanmaurer Jul 24, 2017

Member

thats fine...

@johnou

johnou approved these changes Jul 24, 2017

Correctly handle unsigned int values returned from TCP_INFO
Motivation:

We used an int[] to store all values that are returned in the struct for TCP_INFO which is not good enough as it uses usigned int values.

Modifications:

- Change int[] to long[] and correctly cast values.

Result:

No more truncated values.

@normanmaurer normanmaurer self-assigned this Jul 25, 2017

@normanmaurer normanmaurer added the defect label Jul 25, 2017

@normanmaurer normanmaurer added this to the 4.0.50.Final milestone Jul 25, 2017

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Jul 25, 2017

Member

Cherry-picked into 4.1 (efb2d14) and 4.0 (26d97f5)

Member

normanmaurer commented Jul 25, 2017

Cherry-picked into 4.1 (efb2d14) and 4.0 (26d97f5)

@normanmaurer normanmaurer deleted the fix_tcp_info branch Jul 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment