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

Fix segfault with ATMega16m1 UART simulation #502

Merged
merged 2 commits into from
Aug 11, 2023

Conversation

jack-greenberg
Copy link
Contributor

@jack-greenberg jack-greenberg commented Aug 21, 2022

Previously, when accessing the p->fe.reg in avr_uart.c (in avr_uart_init()) for ATmega16m1, the system segfaulted.

In avr_register_io_read, we see the following lines:

	avr_io_addr_t a = AVR_DATA_TO_IO(addr);
	if (avr->io[a].r.param || avr->io[a].r.c) {

AVR_DATA_TO_IO just subtracts 0x20 from addr. addr is passed in from avr_uart_init as p->fe.reg. But in ATmega16m1, p->fe is not initialized, so it is set to zero. When we subtract 0x20 from 0, and then do avr->io[a], we have an array-out-of-bounds error.

The following previously segfaulted, but no longer does:

./simavr/run_avr -m atmega16m1 -f 4000000 ~/path/to/binary.elf

@jack-greenberg
Copy link
Contributor Author

Possibly addresses #335?

Copy link

@edgar-bonet edgar-bonet left a comment

Choose a reason for hiding this comment

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

the system segfaulted because p->fe is NULL

I don't understand this sentence. p->fe is not a pointer (it's a struct), so it cannot be NULL.

@@ -532,8 +532,11 @@ avr_uart_init(
// status bits
// monitor code that reads the rxc flag, and delay it a bit
avr_register_io_read(avr, p->rxc.raised.reg, avr_uart_status_read, p);
if (p->fe.reg != p->rxc.raised.reg)
avr_register_io_read(avr, p->fe.reg, avr_uart_status_read, p);
if (p->fe.reg) {

Choose a reason for hiding this comment

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

As this test evaluates p->fe.reg, it implicitly assumes this expression is well defined. But if p->fe.reg is well defined, then what is the purpose of the test?

@jack-greenberg
Copy link
Contributor Author

Ah whoops, I mis-wrote this (I actually found this a few months ago but forgot to submit it).

In avr_register_io_read, we see the following lines:

	avr_io_addr_t a = AVR_DATA_TO_IO(addr);
	if (avr->io[a].r.param || avr->io[a].r.c) {

AVR_DATA_TO_IO just subtracts 0x20 from addr. addr is passed in from avr_uart_init as p->fe.reg. But in ATmega16m1, p->fe is not initialized, so it is set to zero. When we subtract 0x20 from 0, and then do avr->io[a], we have an array-out-of-bounds error.

Does that make sense? I will update the PR body.

@edgar-bonet
Copy link

Oh, I see. Now it makes sense to me.

Copy link

@edgar-bonet edgar-bonet left a comment

Choose a reason for hiding this comment

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

I am not sure I am qualified to review this. Nitpicking: the new code uses spaces for indentation, while the rest of the file uses tabs. Otherwise, looks good to me.

@buserror buserror merged commit 13056a1 into buserror:master Aug 11, 2023
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

Successfully merging this pull request may close these issues.

3 participants