Re: [hatari-devel] Natfeats regression?

[Eero Tamminen <oak@xxxxxxxxxxxxxx>]

Hi,

On 17.6.2022 12.49, Thorsten Otto wrote:
> On Freitag, 17. Juni 2022 11:28:14 CEST Eero Tamminen wrote:
> > I've fixed it now:
>
> I haven't actually tested it, but looking at the code there seem to be two
> more problems:
>
> - the inner loop of mem_string_ok only checks the starting address , but never
> advances it. This does not cause problems as long as the atari program does
> not do something stupid, but is certainly not what was intended.
> - if the string is too long (>=NF_MAX_STRING), the function still returns
> failure, without generating an error. That will result in the same behaviour
> as before with an empty string, a freeze on the natfeats instruction.

Good point.


> I think the restriction fo NF_MAX_STRING should be removed.

NF_MAX_STRING is 4KB.  If string is longer, it's definitely a 
non-terminated one (either by accident or design), which will mess up 
terminal and could hide anything else one might want to see.

Note that protections like these are there not just for the devs using 
NatFeats in their own code, but also for other users running such SW.


What do you think of the attached patch?

And would some other exception than bus error be suitable for such issues?


	- Eero

diff --git a/src/debug/natfeats.c b/src/debug/natfeats.c
index 8d4e1634..c72618ef 100644
--- a/src/debug/natfeats.c
+++ b/src/debug/natfeats.c
@@ -66,10 +66,12 @@ static int mem_string_ok(Uint32 addr)
 				return i;
 			}
 		}
+		/* unterminated NF string -> error */
+		M68000_BusError(addr, BUS_ERROR_READ, BUS_ERROR_SIZE_BYTE, BUS_ERROR_ACCESS_DATA, 0);
 		return -1;
 	}
 	for (i = 0; i < NF_MAX_STRING; i++) {
-		if (!STMemory_CheckAreaType(addr, 1, ABFLAG_RAM | ABFLAG_ROM)) {
+		if (!STMemory_CheckAreaType(addr + i, 1, ABFLAG_RAM | ABFLAG_ROM)) {
 			/* ends in invalid area -> error */
 			M68000_BusError(addr, BUS_ERROR_READ, BUS_ERROR_SIZE_BYTE, BUS_ERROR_ACCESS_DATA, 0);
 			return -1;
@@ -78,7 +80,7 @@ static int mem_string_ok(Uint32 addr)
 			return i;
 		}
 	}
-	return -1;
+	assert(false); return 0;  /* should never be reached */
 }
 
 /**