AbstractUdpListener, incorrect length computation about ice4j HOT 6 CLOSED

correct : if (buf == null || buf.length < len || len - off < 20)

This is incorrect. The following values satisfy the condition, and they shouldn't (the described buffer is valid and should be accepted):
buffer.length = 1000
len = 500
off = 500

The existing code handles this correctly.

from ice4j.

The bytes available are buf[j] for off <= j < len. Thus len - off is the number of bytes available, len + off is immaterial.

If len == off (like in the example, which describes a buffer which should be rejected, since no byte is available), buf[off] and the following bytes are out-of-range. This is not caught by the current error condition (line 166).

For the two calls of the method org.ice4j.ice.harvest.AbstractUdpListener.getUfrag(byte[] buf, int off, int len) in question, the following two conditions always hold:

• len <= buf.length
• off == 0

The first condition is reasonable, since the underlying Buffer may come from a pool, and have space for more more than the len bytes actually used.
The second condition shows that off could be removed entirely from the method arguments; otherwise, 0 <= off <= len is a sensible assumption.

Message.decode(...) is called in the method in question, and requires 20 bytes to be available, i.e. buf[off], buf[off+1], ..., buf[off + 19]; thus off + 19 < len must be ensured, which is 20 <= len - off.

Hence the following condition must be ensured: buf != null && len <= buf.length && 20 <= len - off. I confirm thus that the correct error condition (line 166) is, as the logical negation: buf == null || buf.length < len || len - off < 20.

Note that the correct error condition and the current error condition agree if off == 0, as is always the case for now. Also note that len - off < 20 is preferred over len < off + 20, because off + 20 may wrap around and thus be negative.

from ice4j.

@bgrozev You may consider re-opining this issue.

from ice4j.

@flurbi I really like your analysis, however I think your assumptions are wrong.

The bytes available are buf[j] for off <= j < len. Thus len - off is the number of bytes available, len + off is immaterial.

The bytes available are buf[j] for off <= j < off + len because, by design, the number of bytes that are available in the buffer is len and they start at off.

from ice4j.

Sorry, I misinterpreted off, which is always 0 anyway. So perhaps off can be removed? Or otherwise putting your explanation into a one-line comment may help the next reviewer (I had the assignment to review some libraries we use). Also note that buf.length < off + len may involve (theoretically at least) an overflow, a reason for using buf.length - len < off instead.

from ice4j.

In this case 'off' is always 0 and could be removed, but we use this pattern throughout the code, and I think it's pretty standard (e.g. the java DatagramPacket/DatagramSocket interfaces use it), so I don't see it as a problem. If you want to open a PR with improved documentation that would be welcome.

from ice4j.