Fix reading 0 bits

[jmajnert]

I was getting errors where the decoder wanted to decode 0 bits as a field value and fell over. This change adds a graceful failure by emitting and error message

[onf-bot]

Can one of the admins verify this patch?

[gab-arrobo]

ok to test

[gab-arrobo]

@jmajnert, I see only 3 places in the code where an input of numBits = 0 might be given. On all of the other 23 uses/calls of this function, either a numBits different than 0 is given or an if statement is checking whether numBits > 0 before calling the function.

Line 137: If ((pd.bitsOffset) & 0x7) is 8. However, because of the bitwise operation, the result can only be between 0 and 7. Therefore, alignBits will always be between 1 and 8 (i.e., alignBits > 0).

		alignBits := 8 - ((pd.bitsOffset) & 0x7)
                ...
		if val, err := pd.getBitsValue(alignBits); err != nil {

Line 505: If byteLength is 2

	byteLength, err := pd.getBitsValue(8)
        ...
	mantissa, err := pd.getBitsValue(8 * uint(byteLength-1-1))

Line 617: If rawLength is 0

	var rawLength uint
        ...
		rawLength = uint(pd.bytes[pd.byteOffset])
        ...
		rawLength = uint(tempLength)
		rawLength++
        ...
	if rawValue, err := pd.getBitsValue(rawLength * 8); err != nil {

I think it would be better to understand where this issue is coming from. Can you please check whether the issue you are seeing is because of lines 505 or 617?

@eroshiva @SeanCondon, what are your thoughts about this comment?

[jmajnert]

I think it would be better to understand where this issue is coming from. Can you please check whether the issue you are seeing if because of lines 505 or 617?

The data was coming from Viavi simulator. I need to set up the environment to be able to reproduce. This may take a while

[SeanCondon]

@eroshiva @SeanCondon, what are your thoughts about this comment?

Good point @gab-arrobo - I see a debug statement on line 615, so it should be easy to tell if 617 is the source. @jmajnert Maybe it is in previously capture logs?

[jmajnert]

@eroshiva @SeanCondon, what are your thoughts about this comment?

Good point @gab-arrobo - I see a debug statement on line 615, so it should be easy to tell if 617 is the source. @jmajnert Maybe it is in previously capture logs?

I found the logs (attached here). It's line 615

[gab-arrobo]

I found the logs (attached here). It's line 615

@jmajnert, can you please update your PR to accordingly address the issue before calling line 617?

[jmajnert]

@jmajnert, can you please update your PR to accordingly address the issue before calling line 617?

That would mean fixing how rawLength is calculated, right? I'm not an expert on APER. I'd rather not touch this.

[gab-arrobo]

That would mean fixing how rawLength is calculated, right? I'm not an expert on APER. I'd rather not touch this.

Not necessarily, for example in line 960, there is an if statement to call a function only if its argument is > 0. You could do something similar, no?

[jmajnert]

You're proposing guarding against error conditions in multiple call sites instead of doing it once in the callee.
Why would that be a better solution?

[SeanCondon]

I think the benefit is that it gives a more precise error message.

[gab-arrobo]

You're proposing guarding against error conditions in multiple call sites instead of doing it once in the callee. Why would that be a better solution?

@jmajnert, based on the logs you shared, the issue is because pd.bytes[pd.byteOffset] is 0 in line 578. Is it correct for pd.bytes[pd.byteOffset] to be 0? Should we add a return right after line 578 when rawLength = 0?

[jmajnert]

@jmajnert, based on the logs you shared, the issue is because pd.bytes[pd.byteOffset] is 0 in line 578. Is it correct for pd.bytes[pd.byteOffset] to be 0? Should we add a return right after line 578 when rawLength = 0?

That's exactly what I meant! If the calculation in line 578 is incorrect, it should be fixed there. But I'm not an expert and I don't know how to fix it. If the calculation is correct, meaning that input data is malformed, then an if check is enough.
Still I think that GetBitString should check for correctness of its arguments.

[jmajnert]

So far I got these suggestions:

1. Add a rawLength == 0 check before calling pd.getBitsValue(). I added a new PR with this solution here: https://github.com/onosproject/onos-lib-go/pull/247/files
2. "return right after line 578 when rawLength = 0". I added a new PR with this solution here: https://github.com/onosproject/onos-lib-go/pull/248/files

This gives us 3 PRs, they shouldn't be conflicting. Please choose your preferred way of fixing this problem