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? */
}
