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

Ensure localSourceID is positive #141

Merged
merged 1 commit into from Apr 19, 2016

Conversation

Projects
None yet
2 participants
@champtar
Contributor

champtar commented Apr 14, 2016

Math.abs(Long.MIN_VALUE) == Long.MIN_VALUE which is negative
https://docs.oracle.com/javase/7/docs/api/java/lang/Math.html#abs%28long%29

Before this commit
0 <= localSourceID < Integer.MAX_VALUE
or localSourceID == (Long.MIN_VALUE % Integer.MAX_VALUE) == -2
After
0 <= localSourceID <= Integer.MAX_VALUE

This was found using FindBugs
http://findbugs.sourceforge.net/bugDescriptions.html#RV_ABSOLUTE_VALUE_OF_RANDOM_INT

Signed-off-by: Etienne CHAMPETIER champetier.etienne@gmail.com

Ensure localSourceID is positive
Math.abs(Long.MIN_VALUE) == Long.MIN_VALUE which is negative
https://docs.oracle.com/javase/7/docs/api/java/lang/Math.html#abs%28long%29

Before this commit
 0 <= localSourceID < Integer.MAX_VALUE
 or localSourceID == (Long.MIN_VALUE % Integer.MAX_VALUE) == -2
After
 0 <= localSourceID <= Integer.MAX_VALUE

This was found using FindBugs
http://findbugs.sourceforge.net/bugDescriptions.html#RV_ABSOLUTE_VALUE_OF_RANDOM_INT

Signed-off-by: Etienne CHAMPETIER <champetier.etienne@gmail.com>
@bgrozev

This comment has been minimized.

Show comment
Hide comment
@bgrozev

bgrozev Apr 19, 2016

Member

We shouldn't be using Integer.MAX_VALUE here at all, since SSRCs are 32bit. What do you think about changing 0x7FFFFFFF to 0xFFFFFFFF?

Member

bgrozev commented Apr 19, 2016

We shouldn't be using Integer.MAX_VALUE here at all, since SSRCs are 32bit. What do you think about changing 0x7FFFFFFF to 0xFFFFFFFF?

@champtar

This comment has been minimized.

Show comment
Hide comment
@champtar

champtar Apr 19, 2016

Contributor

@bgrozev I agree we could use 0xFFFFFFFF, but this is taking an unnecessary risk (if somewhere we don't handle it well), and 0x7FFFFFFF is enough.

This commit was intended to prevent a close to impossible bug (and remove a line from FindBugs), not to possibly introduce new bugs for no gain :)

Contributor

champtar commented Apr 19, 2016

@bgrozev I agree we could use 0xFFFFFFFF, but this is taking an unnecessary risk (if somewhere we don't handle it well), and 0x7FFFFFFF is enough.

This commit was intended to prevent a close to impossible bug (and remove a line from FindBugs), not to possibly introduce new bugs for no gain :)

@bgrozev

This comment has been minimized.

Show comment
Hide comment
@bgrozev

bgrozev Apr 19, 2016

Member

Agreed, but I think that while we're at it we should also fix the other bug. But I can open a separate PR for that, if you like.

Member

bgrozev commented Apr 19, 2016

Agreed, but I think that while we're at it we should also fix the other bug. But I can open a separate PR for that, if you like.

@champtar

This comment has been minimized.

Show comment
Hide comment
@champtar

champtar Apr 19, 2016

Contributor

Yep, i prefer a separate PR.

This is just an exemple, but if a piece of code is used in Jitsi, we should not break communication between old Jitsi and up to date Jitsi just to find some bug, ie deployment is slow and i prefer to keep back compat

Contributor

champtar commented Apr 19, 2016

Yep, i prefer a separate PR.

This is just an exemple, but if a piece of code is used in Jitsi, we should not break communication between old Jitsi and up to date Jitsi just to find some bug, ie deployment is slow and i prefer to keep back compat

@bgrozev bgrozev merged commit fac4529 into jitsi:master Apr 19, 2016

1 check passed

default Build finished.
Details

@champtar champtar deleted the champtar:math.abs_neg branch Apr 21, 2016

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