Skip to content
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

Types encoding randomly fails if output buffer is not initialized to 0 #233

Open
bonizzifbk opened this issue Mar 30, 2022 · 2 comments
Open

Comments

@bonizzifbk
Copy link

When using the _Encode functions, it is possible that they randomly fail if the output buffer is not previously initialized to 0.

Depending on the specific circumstances, the failure can manifest in two different ways:

  1. both encoding and decoding are reported as successful (return code true), but the decoded data is wrong (see TEST1 and TEST2 in the attached project)
  2. the encoding is successful, but the decoding fails (return code false) (see TEST3)

When initializing the output buffer to 0, everything works fine, but in case of large data types this adds some overhead that is not negligible, especially on low-end embedded targets.

archive.zip

@bonizzifbk
Copy link
Author

The problem seems to originate from two wrong behaviors in file asn1crt/asn1crt_encoding.c.

The first one concerns the function BitStream_AppendNBitZero.

void BitStream_AppendNBitZero(BitStream* pBitStrm, int nbits)
{
	int totalBits = pBitStrm->currentBit + nbits;
	int totalBytes = totalBits / 8;
	pBitStrm->currentBit = totalBits % 8;
	//pBitStrm->currentByte += totalBits / 8;
	if (pBitStrm->currentByte + totalBytes <= pBitStrm->count) {
		pBitStrm->currentByte += totalBytes;
		bitstream_push_data_if_required(pBitStrm);
	} else {
		int extraBytes = pBitStrm->currentByte + totalBytes - pBitStrm->count;
		pBitStrm->currentByte = pBitStrm->count;
		bitstream_push_data_if_required(pBitStrm);
		pBitStrm->currentByte = extraBytes;
	}
}

I think this function, similarly as BitStream_AppendNBitOne, should append as many 0 bits in the pBitStrm->buf as specified by nbits. Although the pBitStrm indexes are updated, there is not an explicit assignment of the buffer to 0.

The second wrong behavior is in BitStream_AppendPartialByte

void BitStream_AppendPartialByte(BitStream* pBitStrm, byte v, byte nbits, flag negate)
{
	int cb = pBitStrm->currentBit;
	int totalBits = cb + nbits;
	int ncb = 8 - cb;
	int totalBitsForNextByte;
	if (negate)
		v = masksb[nbits] & ((byte)~v);
	byte mask1 = (byte)~masksb[ncb];

	if (totalBits <= 8) {
		//static byte masksb[] = { 0x0, 0x1, 0x3, 0x7, 0xF, 0x1F, 0x3F, 0x7F, 0xFF };
		byte mask2 = masksb[8 - totalBits];
		byte mask = mask1 | mask2;
		//e.g. current bit = 3 --> mask =  1110 0000
		//nbits = 3 --> totalBits = 6
		//                         mask=   1110 0000
		//                         and     0000 0011 <- masks[totalBits - 1]
        //	                              -----------
		//					final mask     1110 0011
		pBitStrm->buf[pBitStrm->currentByte] &= mask;
		pBitStrm->buf[pBitStrm->currentByte] |= (byte)(v << (8 - totalBits));
		pBitStrm->currentBit += nbits;
		if (pBitStrm->currentBit == 8) {
			pBitStrm->currentBit = 0;
			pBitStrm->currentByte++;
			bitstream_push_data_if_required(pBitStrm);
		}
	}
	else {
		totalBitsForNextByte = totalBits - 8;
		pBitStrm->buf[pBitStrm->currentByte] &= mask1;
		pBitStrm->buf[pBitStrm->currentByte++] |= (byte)(v >> totalBitsForNextByte);
		bitstream_push_data_if_required(pBitStrm);
		byte mask = (byte)~masksb[8 - totalBitsForNextByte];
		pBitStrm->buf[pBitStrm->currentByte] &= mask;
		pBitStrm->buf[pBitStrm->currentByte] |= (byte)(v << (8 - totalBitsForNextByte));
		pBitStrm->currentBit = totalBitsForNextByte;
	}
	assert(pBitStrm->currentByte * 8 + pBitStrm->currentBit <= pBitStrm->count * 8);

}

In this case the bug is in the else branch when the mask for the next byte assignment is retrieved.

// masksb[] = { 0x0, 0x1, 0x3, 0x7, 0xF, 0x1F, 0x3F, 0x7F, 0xFF };
byte mask = (byte)~masksb[8 - totalBitsForNextByte];
pBitStrm->buf[pBitStrm->currentByte] &= mask;

When the mask is retrieved, it is negated, but this is wrong because with the subsequent &= operation we want to zero with the initial bits of the byte. The mask negation causes to zero the ending bits.

@usr3-1415
Copy link
Collaborator

For the moment, you must always call the BitStream_Init() function before calling the encode function. This function calls memset() and put zeros in the entire buffer.
In near future, we will refactor these functions and this issue will be addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants