Memory issues caused by erroneous sizeof usage and others

[ericonr]

Thankfully, it seems most of these code paths aren't in use at the moment.

Loops through struct with an uint32_t pointer but using sizeof(board_diagnostic_t) directly (without dividing by 4), as if it were an unsigned char pointer (causes a buffer overflow). I think this is wrong in general with aliasing restrictions (a memcpy'd buffer would be more correct), but we do pass -fno-strict-aliasing to the compiler, so that part shouldn't be a concern.

openMMC/modules/fpga_spi.c

Lines 54 to 60 in a631724

	uint32_t *buffer = (uint32_t *)diag;
	/* Send all bytes sequentially, except the last record (FMC slot status), whose address is 0xFF */
	for( i = 0; i < sizeof(board_diagnostic_t)- 1; i++) {
	write_fpga_dword( i, buffer[i] );
	}
	write_fpga_dword( 0xFF, buffer[i] );

Does memset(buffer, size, value) instead of memset(buffer, value, size). I'm not sure this memset needs to be here at all, since it immediately reads the I2C information into the buffer, therefore initializing it (unless the expectation after the I2C read is the presence of a null byte in the buffer). Should probably be replaced by a stack array, by setting a maximum size for the buffer.

openMMC/port/board/afc-bpm/v3_1/ipmi_oem.c

Line 83 in a631724

memset( read_data, read_len, 0 );

Uses sizeof(hpm_page) when hpm_page is a pointer, making most calculations based on it wrong; this is happening all across the file. At least for the first memset call, sizeof(hpm_page) should probably be replaced with PAYLOAD_HPM_PAGE_SIZE, which is the size passed to the allocator.

openMMC/port/board/afc-bpm/v3_1/payload.c

Lines 326 to 344 in a631724

	uint8_t *hpm_page = NULL;
	uint8_t hpm_pg_index;
	uint32_t hpm_page_addr;
	uint8_t payload_hpm_prepare_comp( void )
	{
	/* Initialize variables */
	if (hpm_page != NULL) {
	vPortFree(hpm_page);
	}
	hpm_page = (uint8_t *) pvPortMalloc(PAYLOAD_HPM_PAGE_SIZE);
	if (hpm_page == NULL) {
	/* Malloc failed */
	return IPMI_CC_OUT_OF_SPACE;
	}
	memset(hpm_page, 0xFF, sizeof(hpm_page));

[augustofg]

This was introduced by c692454, it worked before because hpm_page was defined as a constant sized array. I understand the rational of avoiding using pre-allocated memory for rarely used functions, but dynamic allocation comes with its own risks when dealing with systems that lack a MMU. Anyway #137 should fix it.