Donny9 opened a new pull request, #17748:
URL: https://github.com/apache/nuttx/pull/17748
## Summary
fs/file: add reference count protection for stack-allocated file structures
Issue:
When using a stack-allocated file structure, the sequence:
1. file_open() initializes the stack file structure
2. file_mmap() creates memory mapping and increments reference count
3. file_munmap() decrements reference count and may free the file structure
4. file_close() attempts to close already freed structure → crash
Root cause:
The memory mapping operations (fs_reffilep/fs_putfilep) manage reference
counts independently and can free the stack-allocated file structure
prematurely.
Solution:
- Add reference count protection during file_open() for stack-allocated files
- Clear reference count appropriately during file_close()
- This ensures the file structure remains valid throughout its lifetime
-
## Impact
bug fix about stack-allocated file structures
## Testing
```c
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <fcntl.h>
#include <errno.h>
#include <sys/mman.h>
#include <sys/stat.h>
#define TEST_FILE "/tmp/test_refcount.dat"
#define TEST_SIZE 4096
/****************************************************************************
* Private Functions
****************************************************************************/
/****************************************************************************
* Name: test_basic_file_operations
*
* Description:
* Test basic file open/close without mmap to verify baseline functionality
****************************************************************************/
static int test_basic_file_operations(void)
{
struct file filep;
int ret;
printf("\n=== Test 1: Basic file_open/file_close ===\n");
/* Open file */
ret = file_open(&filep, TEST_FILE, O_RDWR | O_CREAT, 0644);
if (ret < 0)
{
printf("FAILED: file_open returned %d\n", ret);
return -1;
}
printf("PASSED: file_open succeeded\n");
/* Close file */
ret = file_close(&filep);
if (ret < 0)
{
printf("FAILED: file_close returned %d\n", ret);
return -1;
}
printf("PASSED: file_close succeeded\n");
return 0;
}
/****************************************************************************
* Name: test_file_with_mmap_munmap
*
* Description:
* Test the critical sequence that exposed the bug:
* 1. file_open() with stack-allocated file structure
* 2. file_mmap() increments reference count
* 3. file_munmap() decrements reference count
* 4. file_close() should succeed without crash
*
* Before fix: file_munmap would free the stack file structure
* After fix: reference count protection prevents premature free
****************************************************************************/
static int test_file_with_mmap_munmap(void)
{
struct file filep;
void *addr;
int ret;
printf("\n=== Test 2: file_open + mmap + munmap + file_close ===\n");
printf("This is the critical test case that exposed the bug\n");
/* Create and write test data to file */
int fd = open(TEST_FILE, O_RDWR | O_CREAT | O_TRUNC, 0644);
if (fd < 0)
{
printf("FAILED: open returned %d, errno=%d\n", fd, errno);
return -1;
}
char buffer[TEST_SIZE];
memset(buffer, 0xAA, TEST_SIZE);
if (write(fd, buffer, TEST_SIZE) != TEST_SIZE)
{
printf("FAILED: write failed\n");
close(fd);
return -1;
}
close(fd);
printf("Step 1: Created test file with %d bytes\n", TEST_SIZE);
/* Open file using file_open() with stack-allocated structure */
ret = file_open(&filep, TEST_FILE, O_RDWR, 0);
if (ret < 0)
{
printf("FAILED: file_open returned %d\n", ret);
return -1;
}
printf("Step 2: file_open succeeded (stack-allocated file structure)\n");
printf(" Initial f_refs should be 1\n");
/* Map the file into memory - this increments reference count */
ret = file_mmap(&filep, NULL, TEST_SIZE, PROT_READ | PROT_WRITE,
MAP_SHARED, 0, &addr);
if (ret < 0)
{
printf("FAILED: file_mmap returned %d\n", ret);
file_close(&filep);
return -1;
}
printf("Step 3: file_mmap succeeded at address %p\n", addr);
printf(" f_refs should now be 2 (open + mmap)\n");
/* Verify we can access the mapped memory */
if (((unsigned char *)addr)[0] != 0xAA)
{
printf("FAILED: mapped memory has wrong content\n");
file_munmap(addr, TEST_SIZE);
file_close(&filep);
return -1;
}
printf("Step 4: Verified mapped memory content\n");
/* Unmap the file - this decrements reference count
* Before fix: This would free the file structure!
* After fix: Reference count protects the structure
*/
ret = file_munmap(addr, TEST_SIZE);
if (ret < 0)
{
printf("FAILED: file_munmap returned %d\n", ret);
file_close(&filep);
return -1;
}
printf("Step 5: file_munmap succeeded\n");
printf(" f_refs should be back to 1\n");
printf(" CRITICAL: file structure must still be valid!\n");
/* Close the file - this is where the crash would occur before the fix
* Before fix: filep already freed, accessing it causes crash
* After fix: filep is still valid and can be safely closed
*/
ret = file_close(&filep);
if (ret < 0)
{
printf("FAILED: file_close returned %d (THIS SHOULD NOT HAPPEN!)\n",
ret);
return -1;
}
printf("Step 6: file_close succeeded WITHOUT CRASH!\n");
printf("PASSED: Reference count protection is working correctly\n");
return 0;
}
/****************************************************************************
* Name: test_multiple_mmap
*
* Description:
* Test multiple mmap operations on the same file to verify
* reference counting works correctly with multiple mappings
****************************************************************************/
static int test_multiple_mmap(void)
{
struct file filep;
void *addr1;
void *addr2;
int ret;
printf("\n=== Test 3: Multiple mmap on same file ===\n");
/* Open file */
ret = file_open(&filep, TEST_FILE, O_RDWR, 0);
if (ret < 0)
{
printf("FAILED: file_open returned %d\n", ret);
return -1;
}
printf("Step 1: file_open succeeded, f_refs = 1\n");
/* First mmap */
ret = file_mmap(&filep, NULL, TEST_SIZE, PROT_READ | PROT_WRITE,
MAP_SHARED, 0, &addr1);
if (ret < 0)
{
printf("FAILED: first file_mmap returned %d\n", ret);
file_close(&filep);
return -1;
}
printf("Step 2: First mmap succeeded, f_refs = 2\n");
/* Second mmap */
ret = file_mmap(&filep, NULL, TEST_SIZE, PROT_READ | PROT_WRITE,
MAP_SHARED, 0, &addr2);
if (ret < 0)
{
printf("FAILED: second file_mmap returned %d\n", ret);
file_munmap(addr1, TEST_SIZE);
file_close(&filep);
return -1;
}
printf("Step 3: Second mmap succeeded, f_refs = 3\n");
/* Unmap first mapping */
ret = file_munmap(addr1, TEST_SIZE);
if (ret < 0)
{
printf("FAILED: first file_munmap returned %d\n", ret);
file_close(&filep);
return -1;
}
printf("Step 4: First munmap succeeded, f_refs = 2\n");
/* Unmap second mapping */
ret = file_munmap(addr2, TEST_SIZE);
if (ret < 0)
{
printf("FAILED: second file_munmap returned %d\n", ret);
file_close(&filep);
return -1;
}
printf("Step 5: Second munmap succeeded, f_refs = 1\n");
/* Close file */
ret = file_close(&filep);
if (ret < 0)
{
printf("FAILED: file_close returned %d\n", ret);
return -1;
}
printf("Step 6: file_close succeeded\n");
printf("PASSED: Multiple mmap reference counting works correctly\n");
return 0;
}
/****************************************************************************
* Name: test_mmap_without_munmap
*
* Description:
* Test closing file while mmap is still active
* This verifies reference count keeps file alive until munmap
****************************************************************************/
static int test_mmap_without_munmap(void)
{
struct file filep;
void *addr;
int ret;
printf("\n=== Test 4: file_close with active mmap ===\n");
/* Open file */
ret = file_open(&filep, TEST_FILE, O_RDWR, 0);
if (ret < 0)
{
printf("FAILED: file_open returned %d\n", ret);
return -1;
}
printf("Step 1: file_open succeeded\n");
/* Map file */
ret = file_mmap(&filep, NULL, TEST_SIZE, PROT_READ | PROT_WRITE,
MAP_SHARED, 0, &addr);
if (ret < 0)
{
printf("FAILED: file_mmap returned %d\n", ret);
file_close(&filep);
return -1;
}
printf("Step 2: file_mmap succeeded, f_refs = 2\n");
/* Close file while mmap is active - should succeed */
ret = file_close(&filep);
if (ret < 0)
{
printf("FAILED: file_close returned %d\n", ret);
file_munmap(addr, TEST_SIZE);
return -1;
}
printf("Step 3: file_close succeeded even with active mmap\n");
printf(" File structure kept alive by mmap reference\n");
/* Access mapped memory - should still work */
if (((unsigned char *)addr)[0] != 0xAA)
{
printf("FAILED: mapped memory lost after file_close\n");
return -1;
}
printf("Step 4: Mapped memory still accessible after file_close\n");
/* Note: In real implementation, we need to track the original filep
* for munmap after file_close. This test may need adaptation based
* on the actual mmap implementation details.
*/
printf("Step 5: Skipping munmap as filep was closed\n");
printf(" (munmap would need separate tracking)\n");
printf("PASSED: Reference counting keeps file alive during mmap\n");
return 0;
}
/****************************************************************************
* Name: test_error_path
*
* Description:
* Test error paths to ensure reference counting works correctly
* even when operations fail
****************************************************************************/
static int test_error_path(void)
{
struct file filep;
void *addr;
int ret;
printf("\n=== Test 5: Error path handling ===\n");
/* Open non-existent file - should fail */
ret = file_open(&filep, "/nonexistent/file", O_RDONLY, 0);
if (ret >= 0)
{
printf("FAILED: file_open should have failed but succeeded\n");
file_close(&filep);
return -1;
}
printf("Step 1: file_open correctly failed for non-existent file\n");
/* Open valid file */
ret = file_open(&filep, TEST_FILE, O_RDWR, 0);
if (ret < 0)
{
printf("FAILED: file_open returned %d\n", ret);
return -1;
}
printf("Step 2: file_open succeeded for valid file\n");
/* Try to mmap with invalid parameters */
ret = file_mmap(&filep, NULL, 0, PROT_READ, MAP_SHARED, 0, &addr);
if (ret >= 0)
{
printf("WARNING: file_mmap with size 0 succeeded (may be valid)\n");
file_munmap(addr, 0);
}
else
{
printf("Step 3: file_mmap correctly failed with invalid size\n");
}
/* Close file - should succeed even if mmap failed */
ret = file_close(&filep);
if (ret < 0)
{
printf("FAILED: file_close returned %d\n", ret);
return -1;
}
printf("Step 4: file_close succeeded after mmap failure\n");
printf("PASSED: Error path handling works correctly\n");
return 0;
}
/****************************************************************************
* Public Functions
****************************************************************************/
int main(int argc, char *argv[])
{
int failures = 0;
printf("========================================\n");
printf("File Reference Count Protection Test\n");
printf("========================================\n");
printf("\nThis test verifies the fix for stack-allocated\n");
printf("file structure reference count protection.\n");
printf("\nBug scenario:\n");
printf("1. file_open() initializes stack file structure\n");
printf("2. file_mmap() increments reference count\n");
printf("3. file_munmap() decrements and may free structure\n");
printf("4. file_close() crashes on freed structure\n");
printf("\nThe fix adds reference count protection to prevent\n");
printf("premature deallocation of stack-allocated files.\n");
/* Run all tests */
if (test_basic_file_operations() < 0)
{
failures++;
}
if (test_file_with_mmap_munmap() < 0)
{
failures++;
}
if (test_multiple_mmap() < 0)
{
failures++;
}
if (test_mmap_without_munmap() < 0)
{
failures++;
}
if (test_error_path() < 0)
{
failures++;
}
/* Cleanup */
unlink(TEST_FILE);
/* Summary */
printf("\n========================================\n");
if (failures == 0)
{
printf("✓ All tests PASSED!\n");
printf("Reference count protection is working correctly.\n");
}
else
{
printf("✗ %d test(s) FAILED!\n", failures);
printf("Reference count protection may have issues.\n");
}
printf("========================================\n");
return failures;
}
```
<img width="635" height="1196" alt="image"
src="https://github.com/user-attachments/assets/c4e3aa47-7014-442e-83ec-5e5b76df1e94";
/>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
