martijnvanbrummelen / nwipe

nwipe secure disk eraser
GNU General Public License v2.0
799 stars 86 forks source link

Fixed memory leak in line 328, writing 1 byte beyond allocated memory. #562

Closed Knogle closed 8 months ago

Knogle commented 8 months ago

Based on the Valgrind output, I noticed an "Invalid read of size 1" error occurring in the nwipe_random_pass function. After inspecting the relevant portions of the source code, it seems like the issue is related to an off-by-one error where my code attempts to access memory just outside the allocated buffer.

Thread 4:
==6062== Invalid read of size 1
==6062==    at 0x4117BC: nwipe_random_pass (pass.c:328)
==6062==    by 0x415EFC: nwipe_runmethod (method.c:934)
==6062==    by 0x417029: nwipe_random (method.c:742)
==6062==    by 0x4E98946: start_thread (pthread_create.c:444)
==6062==    by 0x4F1E873: clone (clone.S:100)
==6062==  Address 0x539da20 is 0 bytes after a block of size 4,096 alloc'd
==6062==    at 0x4849E60: calloc (vg_replace_malloc.c:1595)
==6062==    by 0x4115FE: nwipe_random_pass (pass.c:268)
==6062==    by 0x415EFC: nwipe_runmethod (method.c:934)
==6062==    by 0x417029: nwipe_random (method.c:742)
==6062==    by 0x4E98946: start_thread (pthread_create.c:444)
==6062==    by 0x4F1E873: clone (clone.S:100)

Here's the problematic part of the code around line 328:

/* For the first block only, check the prng actually wrote something to the buffer */
if( z == c->device_size )
{
    idx = c->device_stat.st_blksize;
    while( idx > 0 )
    {
        if( b[idx] != 0 )
        {
            nwipe_log( NWIPE_LOG_NOTICE, "prng stream is active" );
            break;

This issue arises because I set idx to c->device_stat.st_blksize, intending to access the last element of the buffer b, but actually end up attempting to access one position beyond it, due to the zero-based indexing of arrays in C. This results in an invalid memory access, as flagged by Valgrind.

To resolve this, I should adjust the index to ensure it remains within the bounds of the allocated memory. The corrected approach would be to decrement the starting index by one, ensuring it points to the last valid element of the array:

idx = c->device_stat.st_blksize - 1;
while( idx > 0 )
{
    if( b[idx] != 0 )
    {
        nwipe_log( NWIPE_LOG_NOTICE, "prng stream is active" );
        break;
    }

This adjustment prevents accessing memory outside of the allocated range, thereby resolving the Valgrind error.