Fantastic exchange from DConf

[Patrick Schluter via Digitalmars-d]

On Tuesday, 9 May 2017 at 06:15:12 UTC, H. S. Teoh wrote:
>
> 	int my_func(mytype_t *input, outbuf_t *output_buf,
> 	            char *buffer, int size)
> 	{
> 		/* Typical lazy way of null-checking (that will blow up
> 		 * later) */
> 		myhandle_t *h = input ? input->handle : 0;
> 		writer_t *w = output_buf ? output_buf->writer : 0;
> 		char *block = (char *)malloc(size);

Hey, you've been outed as a C++ programmer. A real C programmer 
never casts a void *. In that specific case, casting away the 
malloc() return can mask a nasty bug. If you have forgotten to 
include the header declaring the function, the compiler would 
assume an int returning function and the cast would suppress the 
righteous warning message of the compiler. On 64 bit machines the 
returned pointer would be truncated to the lower half. 
Unfortunately on Linux, as the heap starts in the lower 4 GiB of 
address space, the code would run for a long time before it 
crashed. On Solaris-SPARC it would crash directly as binaries are 
loaded address 0x1_0000_0000 of the address space.

> 		FILE *fp;
> 		int i;
>
> 		if (!buffer)
> 			return -1; /* typical useless error return code */
> 				/* (also, memory leak) */
>
> 		if (h->control_block) { /* oops, possible null deref */
> 			fp = fopen("blah", "w");
> 			if (!fp)
> 				return -1; /* oops, memory leak */
> 		}
> 		if (w->buffered) { /* oops, possible null deref */
> 			strncpy(buffer, input->data, size); /* oops, unterminated 
> string */
> 			if (w->write(buffer, size) != 0)
> 				/* hmm, is 0 the error status, or is it -1? */
> 				/* also, what if w->write == null? */

Or is it inspired by fwrite, which returns the number of written 
records? In that case 0 return might be an error or not, depends 
on size.

> 			{
> 				return -1; /* oops, memory leak AND file
> 						descriptor leak */
> 			}
> 		}
> 		for (i = 0; i <= input->size; i++) {	/* oops, off-by-1 error 
> */
> 			... /* more nauseating nasty stuff here */
> 			if (error)
> 				goto EXIT;
> 			... /* ad nauseum */
> 		}
> 	EXIT:
> 		if (fp) fclose(fp);	/* oops, uninitialized ptr deref */

Worse, you didn't check the return of fclose() on writing FILE. 
fclose() can fail if the disk was full. As the FILE is buffered, 
the last fwrite might not have flushed it yet. So it is the 
fclose() that will try to write the last block and that can fail, 
but the app wouldn't be able to even report it.

> 		free(block);
>
> 		/* Typical lazy way of evading more tedious `if
> 		 * (func(...) == error) goto EXIT;` style code, which
> 		 * ends up being even more error-prone */
> 		return error ? -1 : w->num_bytes_written();
> 			/* oops, what if w or w->num_bytes_written is
> 			 * null? */
> 	}
>