[BUG] Static Global State Variables Used for Multiple Connections

[jlsantiago0]

If you use the SRT library with multiple SRT connections in it. The global static state variables in srt/haicrypt/hcrypt_xpt_srt.c are used by all encrypted SRT connections. For instance if you have an application that is inputing and encrypted SRT stream and outputing an encrypted SRT stream. The following global static state variables are used by all of them at the same time:

srt/haicrypt/hcrypt_xpt_srt.c :

// Line: 68
static hcrypt_MsgInfo _hcMsg_SRT_MsgInfo;
// Line: 156
static hcrypt_MsgInfo _hcMsg_SRT_MsgInfo;

This can be observed with the srt-live-transmit application where you ijnput an ecrypted SRT source stream and output and encrypted SRT stream.

[jlsantiago0]

NOTE: These variables are collapsed into a single common global variable. Since they have the same name. Even if they had different names, there would also be interference.

[maxsharabayko]

So if I follow the logic we just need to delete one definition. @jeandube could you please confirm?
The structure is used to initialize functions pointers - ok for multiple threads.
pfx_len and hdr_len are also initialized once and do not change. Worth making them const, but not possible in C, I assume...

	_hcMsg_SRT_MsgInfo.hdr_len      = HCRYPT_MSG_SRT_HDR_SZ;
	_hcMsg_SRT_MsgInfo.pfx_len      = HCRYPT_MSG_SRT_PFX_SZ;

[maxsharabayko]

Hm, this function actually does not need a static variable. It must initialize and return a local variable.

hcrypt_MsgInfo *hcryptMsg_SRT_MsgInfo(void)
{
	_hcMsg_SRT_MsgInfo.hdr_len      = HCRYPT_MSG_SRT_HDR_SZ;
	_hcMsg_SRT_MsgInfo.pfx_len      = HCRYPT_MSG_SRT_PFX_SZ;
	_hcMsg_SRT_MsgInfo.getKeyFlags  = hcryptMsg_SRT_GetKeyFlags;
	_hcMsg_SRT_MsgInfo.getPki       = hcryptMsg_SRT_GetPki;
	_hcMsg_SRT_MsgInfo.setPki       = hcryptMsg_SRT_SetPki;
	_hcMsg_SRT_MsgInfo.resetCache   = hcryptMsg_SRT_ResetCache;
	_hcMsg_SRT_MsgInfo.indexMsg     = hcryptMsg_SRT_IndexMsg;
	_hcMsg_SRT_MsgInfo.parseMsg     = hcryptMsg_SRT_ParseMsg;

	return(&_hcMsg_SRT_MsgInfo);
}

Update:
Nope, I am wrong. It returns a pointer :)

[maxsharabayko]

Should be something like this, but hcryptMsg_SRT_ParseMsg uses a pointer to a static variable, so problems with .parseMsg = hcryptMsg_SRT_ParseMsg.

static const hcrypt_MsgInfo _hcMsg_SRT_MsgInfo = {
	.hdr_len     = HCRYPT_MSG_SRT_HDR_SZ,
	.pfx_len     = HCRYPT_MSG_SRT_PFX_SZ,
	.getKeyFlags = hcryptMsg_SRT_GetKeyFlags,
	.getPki      = hcryptMsg_SRT_GetPki,
	.setPki      = hcryptMsg_SRT_SetPki,
	.resetCache  = hcryptMsg_SRT_ResetCache,
	.indexMsg    = hcryptMsg_SRT_IndexMsg,
	.parseMsg    = hcryptMsg_SRT_ParseMsg
};

const hcrypt_MsgInfo* hcryptMsg_SRT_InitMsgInfo(void)
{
	return (&_hcMsg_SRT_MsgInfo);
}

[ethouris]

I think this whole code would be better off if this is translated to C++ and these assignments are replaced by virtual methods.

[jeandube]

This structure is constant for all SRT streams. Reminiscence of the time HaiCrypt lib was also used on TS/UDP with a different header format. Remember this is from a "C" programmer trying to do C++.

[ethouris]

That's what I mean. This code uses the usual concept of "virtual calls" and dynamic code configuration, so values contained there are propertied to the build configuration of the SRT library, not to the stream or even application (unless we have concurrent applications using different variants of SRT, but I doubt this is possible, or anyhow tolerable).