[jitsi-dev] [jitsi/libjitsi] Ensure localSourceID is positive (#141)


#1

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

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>
You can view, comment on, or merge this pull request online at:

  https://github.com/jitsi/libjitsi/pull/141

-- Commit Summary --

  * Ensure localSourceID is positive

-- File Changes --

    M src/org/jitsi/impl/neomedia/MediaStreamImpl.java (3)

-- Patch Links --

https://github.com/jitsi/libjitsi/pull/141.patch
https://github.com/jitsi/libjitsi/pull/141.diff

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/141


#2

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

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/141#issuecomment-211949724


#3

@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 :slight_smile:

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/141#issuecomment-211970181


#4

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.

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/141#issuecomment-211980783


#5

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

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/141#issuecomment-211988335


#6

Merged #141.

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/141#event-633336334