Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Mar 26, 2024

Conversation

Knogle
Copy link
Contributor

@Knogle Knogle commented Mar 26, 2024

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.

@PartialVolume PartialVolume merged commit 8ad4fe2 into martijnvanbrummelen:master Mar 26, 2024
2 checks passed
Knogle pushed a commit to Knogle/nwipe that referenced this pull request Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants