Fix memory leaks reported by Address Sanitizer

While these leaks are "not important" (the only large buffer must persist for the full lifetime of the program) it's nice to be able to eliminate them and get a clean run under debugging tools.
Allocate buffer of correct size
2022-09-15 11:30:13 -05:00 · 2022-09-15 11:30:13 -05:00 · 2022-09-15 11:30:12 -05:00 · 2022-09-15 11:30:12 -05:00 · 2022-09-15 10:55:44 -05:00 · 2022-03-10 21:48:26 -06:00
8 changed files with 42 additions and 4375 deletions
--- a/asm/Makefile
+++ b/asm/Makefile
@ -1,5 +1,5 @@
 CL = cl65
-CL_FLAGS = -t none --listing --list-bytes 100
+CL_FLAGS = -t none

 ASRC = $(shell echo *.s)
 AOBJ = $(ASRC:%.s=%.o)
--- a/asm/diskload3.s
+++ b/asm/diskload3.s
@ -143,7 +143,7 @@ inflateFixedBlock_setCodeLengths:
 	cpy	#LENGTH_SYMBOLS
 	bcs	inflateFixedBlock_setControlCodeLength
 	cpy	#24
-	adc	#2-DISTANCE_TREE
+	adc	#<(2-DISTANCE_TREE)
 inflateFixedBlock_setControlCodeLength:
 	sta	controlSymbolCodeLength,y
 inflateFixedBlock_noControlSymbol:
@ -348,7 +348,7 @@ buildHuffmanTree_noControlSymbol:
 	bne	buildHuffmanTree_countCodeLengths
 ; Calculate offsets of symbols sorted by code length
 ;	lda	#0
-	ldx	#-3*TREE_SIZE
+	ldx	#<(-3*TREE_SIZE)
 buildHuffmanTree_calculateOffsets:
 	sta	nBitCode_literalOffset+3*TREE_SIZE-$100,x
 ;;	add	nBitCode_literalCount+3*TREE_SIZE-$100,x
--- a/asm/inflate.s
+++ b/asm/inflate.s
@ -141,7 +141,7 @@ inflateFixedBlock_setCodeLengths:
 	cpy	#LENGTH_SYMBOLS
 	bcs	inflateFixedBlock_setControlCodeLength
 	cpy	#24
-	adc	#2-DISTANCE_TREE
+	adc	#<(2-DISTANCE_TREE)
 inflateFixedBlock_setControlCodeLength:
 	sta	controlSymbolCodeLength,y
 inflateFixedBlock_noControlSymbol:
@ -346,7 +346,7 @@ buildHuffmanTree_noControlSymbol:
 	bne	buildHuffmanTree_countCodeLengths
 ; Calculate offsets of symbols sorted by code length
 ;	lda	#0
-	ldx	#-3*TREE_SIZE
+	ldx	#<(-3*TREE_SIZE)
 buildHuffmanTree_calculateOffsets:
 	sta	nBitCode_literalOffset+3*TREE_SIZE-$100,x
 ;;	add	nBitCode_literalCount+3*TREE_SIZE-$100,x
--- a/c2t-96h.c
+++ b/c2t-96h.c
@ -271,7 +271,7 @@ int main(int argc, char **argv)

 	fprintf(stderr,"\n");
 	for(i=optind;i<argc-fileoutput;i++) {
-		char start[5];
+		char *start;
 		unsigned char b, *data;
 		int j, k, inputtype=BINARY;
 		segment *tmp;
@ -285,18 +285,17 @@ int main(int argc, char **argv)

 		k=0;
 		for(j=0;j<strlen(argv[i]);j++) {
-			if(argv[i][j] == ',')
+			if(argv[i][j] == ',') {
+				j++;
 				break;
+			}
 			segments[numseg].filename[k++]=argv[i][j];
 		}
 		segments[numseg].filename[k] = '\0';
 		// TODO: store as basename, check for MINGW compat

-		k=0;j++;
-		for(;j<strlen(argv[i]);j++)
-			start[k++]=argv[i][j];
-		start[k] = '\0';
-		if(k == 0)
+		start = &argv[i][j];
+		if(!*start)
 			segments[numseg].start = -1;
 		else
 			segments[numseg].start = (int)strtol(start, (char **)NULL, 16);
@ -320,6 +319,7 @@ int main(int argc, char **argv)
 			if(strcmp(ext,"PO") == 0)
 				inputtype = DSK;
 		}
+		free(ext);

 //TODO: Windows needs "rb", check UNIX/Linux

@ -526,6 +526,7 @@ int main(int argc, char **argv)
 				return 1;
 			}
 		}
+		free(ext);
 	}
 	else {
 /*
@ -1471,6 +1472,11 @@ int main(int argc, char **argv)
 		Write_WAVE(ofp,buf.sound,buf.length,rate,bits,amp);

 	fclose(ofp);
+	free(buf.sound);
+	for(i=0;i<numseg;i++) {
+		free(segments[i].data);
+	}
+	free(segments);
 	return 0;
 }

@ -1562,7 +1568,7 @@ char *getext(char *filename)
 	if(sp == strlen(filename) || sp == 0)
 		return(NULL);

-	if((rval = (char *)malloc(sp * sizeof(char))) == NULL)
+	if((rval = (char *)malloc((sp + 1) * sizeof(char))) == NULL)
 		; //do error code

 	rval[sp] = '\0';
--- a/c2t.c
+++ b/c2t.c
@ -228,7 +228,7 @@ int main(int argc, char **argv)

 	fprintf(stderr,"\n");
 	for(i=optind;i<argc-fileoutput;i++) {
-		char start[5];
+		char *start;
 		unsigned char b, *data;
 		int j, k, inputtype=BINARY;
 		segment *tmp;
@ -242,18 +242,17 @@ int main(int argc, char **argv)

 		k=0;
 		for(j=0;j<strlen(argv[i]);j++) {
-			if(argv[i][j] == ',')
+			if(argv[i][j] == ',') {
+				j++; // skip over comma, if present
 				break;
+			}
 			segments[numseg].filename[k++]=argv[i][j];
 		}
 		segments[numseg].filename[k] = '\0';
 		// TODO: store as basename, check for MINGW compat

-		k=0;j++;
-		for(;j<strlen(argv[i]);j++)
-			start[k++]=argv[i][j];
-		start[k] = '\0';
-		if(k == 0)
+		start = &argv[i][j]; // points at the start address or at '\0' if no start address given
+		if(!*start)
 			segments[numseg].start = -1;
 		else
 			segments[numseg].start = (int)strtol(start, (char **)NULL, 16);
@ -320,6 +319,7 @@ int main(int argc, char **argv)
 				fprintf(stderr,"Reading %s, type %s, segment %d, start: ",segments[numseg].filename,filetypes[inputtype],numseg+1);
 			}
 		}
+		free(ext);

 		if(inputtype == BINARY) {
 			if(segments[numseg].start == -1) {
@ -437,6 +437,7 @@ int main(int argc, char **argv)
 				return 1;
 			}
 		}
+		free(ext);
 	}
 	else {
 /*
@ -1353,6 +1354,11 @@ int main(int argc, char **argv)
 		Write_WAVE(ofp,output,outputlength,rate,bits,amp);

 	fclose(ofp);
+	free(output);
+	for(i=0;i<numseg;i++) {
+		free(segments[i].data);
+	}
+	free(segments);
 	return 0;
 }

@ -1423,7 +1429,7 @@ char *getext(char *filename)
 	if(sp == strlen(filename) || sp == 0)
 		return(NULL);

-	if((rval = (char *)malloc(sp * sizeof(char))) == NULL)
+	if((rval = (char *)malloc((1 + sp) * sizeof(char))) == NULL)
 		; //do error code

 	rval[sp] = '\0';
--- a/c2t.h
+++ b/c2t.h
--- a/1
+++ b/1
@ -1,4 +1,5 @@
 #!/bin/bash
+set -e

 header()
 {
--- a/miniz.h
+++ b/miniz.h
@ -1410,7 +1410,10 @@ tinfl_status tinfl_decompress(tinfl_decompressor *r, const mz_uint8 *pIn_buf_nex
      {
        mz_uint8 *p = r->m_tables[0].m_code_size; mz_uint i;
        r->m_table_sizes[0] = 288; r->m_table_sizes[1] = 32; TINFL_MEMSET(r->m_tables[1].m_code_size, 5, 32);
-        for ( i = 0; i <= 143; ++i) *p++ = 8; for ( ; i <= 255; ++i) *p++ = 9; for ( ; i <= 279; ++i) *p++ = 7; for ( ; i <= 287; ++i) *p++ = 8;
+        for ( i = 0; i <= 143; ++i) *p++ = 8;
+        for ( ; i <= 255; ++i) *p++ = 9;
+        for ( ; i <= 279; ++i) *p++ = 7;
+        for ( ; i <= 287; ++i) *p++ = 8;
      }
      else
      {
@ -2195,7 +2198,10 @@ static __forceinline void tdefl_find_match(tdefl_compressor *d, mz_uint lookahea
        if (TDEFL_READ_UNALIGNED_WORD(&d->m_dict[probe_pos + match_len - 1]) == c01) break;
      TDEFL_PROBE; TDEFL_PROBE; TDEFL_PROBE;
    }
-    if (!dist) break; q = (const mz_uint16*)(d->m_dict + probe_pos); if (TDEFL_READ_UNALIGNED_WORD(q) != s01) continue; p = s; probe_len = 32;
+    if (!dist) break;
+    q = (const mz_uint16*)(d->m_dict + probe_pos);
+    if (TDEFL_READ_UNALIGNED_WORD(q) != s01) continue;
+    p = s; probe_len = 32;
    do { } while ( (TDEFL_READ_UNALIGNED_WORD(++p) == TDEFL_READ_UNALIGNED_WORD(++q)) && (TDEFL_READ_UNALIGNED_WORD(++p) == TDEFL_READ_UNALIGNED_WORD(++q)) &&
                   (TDEFL_READ_UNALIGNED_WORD(++p) == TDEFL_READ_UNALIGNED_WORD(++q)) && (TDEFL_READ_UNALIGNED_WORD(++p) == TDEFL_READ_UNALIGNED_WORD(++q)) && (--probe_len > 0) );
    if (!probe_len)
Author	SHA1	Message	Date
Jeff Epler	43c9c13b3b	Fix memory leaks reported by Address Sanitizer While these leaks are "not important" (the only large buffer must persist for the full lifetime of the program) it's nice to be able to eliminate them and get a clean run under debugging tools.	2022-09-15 11:30:13 -05:00
Jeff Epler	d02470f190	Allocate buffer of correct size Before this, the buffer was too small. Address sanitizer detected the problem as follows: ``` $ bin/c2t-96h -2bc8 tests/images/super_puckman,800 puckman.aif Reading tests/images/super_puckman, type binary, segment 1, start: 0x0800, length: 30719 ================================================================= ==1181528==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000000013 at pc 0x55e07e680ff5 bp 0x7ffc0cf3bca0 sp 0x7ffc0cf3bc98 WRITE of size 1 at 0x602000000013 thread T0 #0 0x55e07e680ff4 in getext /home/jepler/src/c2t/c2t-96h.c:1567 #1 0x55e07e651933 in main /home/jepler/src/c2t/c2t-96h.c:512 #2 0x7f51a5b39d09 in __libc_start_main ../csu/libc-start.c:308 #3 0x55e07e6573d9 in _start (/home/jepler/src/c2t/bin/c2t-96h+0x113d9) 0x602000000013 is located 0 bytes to the right of 3-byte region [0x602000000010,0x602000000013) allocated by thread T0 here: #0 0x7f51a5ed8e8f in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:145 #1 0x55e07e680e7b in getext /home/jepler/src/c2t/c2t-96h.c:1564 #2 0x55e07e687722 (/home/jepler/src/c2t/bin/c2t-96h+0x41722) SUMMARY: AddressSanitizer: heap-buffer-overflow /home/jepler/src/c2t/c2t-96h.c:1567 in getext Shadow bytes around the buggy address: 0x0c047fff7fb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c047fff7fc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c047fff7fd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c047fff7fe0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c047fff7ff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 =>0x0c047fff8000: fa fa[03]fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c047fff8010: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c047fff8020: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c047fff8030: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c047fff8040: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c047fff8050: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb Shadow gap: cc ==1181528==ABORTING ``` In order for it to be OK to access `rval[sp]`, it has to be allocated with (sp+1) elements.	2022-09-15 11:30:13 -05:00
Jeff Epler	e9f80a2e42	Get rid of unneeded fixed-length buffer gcc 10.2.1 noticed a potential problem with buffer handling: ``` c2t.c:255:12: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=] 255 \| start[k] = '\0'; \| ~~~~~~~~~^~~~~~ c2t.c:231:8: note: at offset 5 to object ‘start’ with size 5 declared here 231 \| char start[5]; \| ^~~~~ ``` By changing `start` to point into the argument string (either immediately after the "," or to the end of the string), the fixed length buffer is no longer needed. This causes no change in behavior for reasonable start addresses (hex values of up to 32 bits), though it could fix an out of bounds memory write condition if an invalid commandline was specified, like `example,12345`.	2022-09-15 11:30:12 -05:00
Jeff Epler	5a697d447d	Fix -Wmisleading-indentation diagnostics Using gcc 10.2.1, diagnostics are reported similar to the following: ``` miniz.h:1413:9: warning: this ‘for’ clause does not guard... [-Wmisleading-indentation] 1413 \| for ( i = 0; i <= 143; ++i) p++ = 8; for ( ; i <= 255; ++i) p++ = 9; for ( ; i <= 279; ++i) p++ = 7; for ( ; i <= 287; ++i) p++ = 8; \| ^~~ miniz.h:1413:47: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the ‘for’ 1413 \| for ( i = 0; i <= 143; ++i) p++ = 8; for ( ; i <= 255; ++i) p++ = 9; for ( ; i <= 279; ++i) p++ = 7; for ( ; i <= 287; ++i) p++ = 8; \| ^~~ ``` This change resolves the diagnostics by adding newlines, so that the indentation of the code matches the compiler's interpretation.	2022-09-15 11:30:12 -05:00
Jeff Epler	614b8ecd3e	Fix negative values compatibly with cc65 2.13.3 .. by using the "unary low byte" operator. The object code in the generated c2t.h matches what was generated before, and a local test with cl65 V2.13.3 - (C) Copyright 1998-2009 Ullrich von Bassewitz also succeeded and matched what was generated before.	2022-09-15 10:55:44 -05:00
Jeff Epler	b459cc1b2a	Don't store this generated file in git it can be automatically regenerated by `make`. However, its presence in git may prevent `make` from rebuilding it, concealing problems with the software build process.	2022-03-10 21:48:26 -06:00
Jeff Epler	29c575b3f4	Fix negative constants not accepted by ca65 V2.18 - Debian 2.19-1 These were clearly accepted in the past, but no longer. The resulting generated c2t.h does not differ in the generated object code. With this change, a locally built `c2t-96h` succeeds.	2022-03-10 21:47:40 -06:00
Jeff Epler	17dc28d9d2	makeheader: exit after any subprocesses error This allows the `make` process to terminate when `makeheader` fails, for example due to the "Don't know what to do with '100'" problem.	2022-03-10 21:45:51 -06:00
Jeff Epler	cbd4013ac1	Remove flags not understood by cl65 V2.18 - Debian 2.19-1 This combination of arguments produces the message ``` cl65: Don't know what to do with '100' ``` which cascades into additional build problems.	2022-03-10 21:45:12 -06:00