Skip to content

Commit

Permalink
Fixed potential buffer overruns when adding too many .ini array eleme…
Browse files Browse the repository at this point in the history
…nts. (#923)

Co-authored-by: Steve Robb <[email protected]>
  • Loading branch information
steverobb and Steve Robb authored Oct 10, 2024
1 parent 042ad82 commit 59916eb
Showing 1 changed file with 7 additions and 4 deletions.
11 changes: 7 additions & 4 deletions cfg.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ static const ini_var_t ini_vars[] =
{ "SHARED_FOLDER", (void*)(&(cfg.shared_folder)), STRING, 0, sizeof(cfg.shared_folder) - 1 },
{ "NO_MERGE_VID", (void*)(&(cfg.no_merge_vid)), HEX16, 0, 0xFFFF },
{ "NO_MERGE_PID", (void*)(&(cfg.no_merge_pid)), HEX16, 0, 0xFFFF },
{ "NO_MERGE_VIDPID", (void*)(cfg.no_merge_vidpid), HEX32ARR, 0, 0xFFFFFFFF },
{ "NO_MERGE_VIDPID", (void*)(cfg.no_merge_vidpid), HEX32ARR, 0, sizeof(cfg.no_merge_vidpid) / sizeof(cfg.no_merge_vidpid[0]) },
{ "CUSTOM_ASPECT_RATIO_1", (void*)(&(cfg.custom_aspect_ratio[0])), STRING, 0, sizeof(cfg.custom_aspect_ratio[0]) - 1 },
{ "CUSTOM_ASPECT_RATIO_2", (void*)(&(cfg.custom_aspect_ratio[1])), STRING, 0, sizeof(cfg.custom_aspect_ratio[1]) - 1 },
{ "SPINNER_VID", (void*)(&(cfg.spinner_vid)), HEX16, 0, 0xFFFF },
Expand Down Expand Up @@ -126,7 +126,7 @@ static const ini_var_t ini_vars[] =
{ "HDR_AVG_NITS", (void*)(&(cfg.hdr_avg_nits)), UINT16, 100, 10000 },
{ "VGA_MODE", (void*)(&(cfg.vga_mode)), STRING, 0, sizeof(cfg.vga_mode) - 1 },
{ "NTSC_MODE", (void *)(&(cfg.ntsc_mode)), UINT8, 0, 2 },
{ "CONTROLLER_UNIQUE_MAPPING", (void *)(cfg.controller_unique_mapping), UINT32ARR, 0, 0xFFFFFFFF },
{ "CONTROLLER_UNIQUE_MAPPING", (void *)(cfg.controller_unique_mapping), UINT32ARR, 0, sizeof(cfg.controller_unique_mapping) / sizeof(cfg.controller_unique_mapping[0]) },
{ "OSD_LOCK", (void*)(&(cfg.osd_lock)), STRING, 0, sizeof(cfg.osd_lock) - 1 },
{ "OSD_LOCK_TIME", (void*)(&(cfg.osd_lock_time)), UINT16, 0, 60 },
{ "DEBUG", (void *)(&(cfg.debug)), UINT8, 0, 1 },
Expand Down Expand Up @@ -410,8 +410,11 @@ static void ini_parse_var(char* buf)
}

uint32_t *arr = (uint32_t*)var->var;
uint32_t pos = ++arr[0];
ini_parse_numeric(var, &buf[i], &arr[pos]);
if (arr[0] < var->max)
{
uint32_t pos = ++arr[0];
ini_parse_numeric(var, &buf[i], &arr[pos]);
}
}
break;

Expand Down

3 comments on commit 59916eb

@javier-rodas
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @steverobb (and @sorgelig), a colleague of mine is getting an "INI Error" since the last time he run update_all and I'm wondering if this 59916eb change affects the behaviour of the "no_merge_vidpid=0x584d474d" variable at the MiSTer.ini file, which was working before, and we think it has the right syntax. Please see the 3 attached images:

Thanks.

Image
Image
Image

@steverobb
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @javier-rodas,
Yeah, it looks like I've broken it by reusing the max field for the array size which is used for the max string length in arrays. There is no array size field. The effect is that the valid NO_MERGE_VIDPID values range from 0 to 256.
I'll get a PR in for a fix. Sorry for the disruption.
Steve

@rsn8887
Copy link
Contributor

@rsn8887 rsn8887 commented on 59916eb Jan 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be reverted

Please sign in to comment.