[DO NOT MERGE] Dummy PR to check for coverity integration#13
[DO NOT MERGE] Dummy PR to check for coverity integration#13sowmiyachelliah wants to merge 1 commit intodevelopfrom
Conversation
| { | ||
| fprintf(stderr,"\nCreateDnsmasqServerConf() Error opening file %s \n", RESOLV_CONF); | ||
| return; | ||
| /* LOW SEVERITY ISSUE: Dead code - unreachable after return */ |
Check failure
Code scanning / CodeQL
Likely overrunning write Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
In general, buffer overflows from strcpy occur when the source string can be larger than the destination buffer. The fix is to either (a) ensure the destination is sized to hold the largest possible source plus the terminating null, or (b) replace strcpy with a bounded copy function (e.g., strncpy, strcpy_s, or strlcpy where available) that limits the number of bytes written to the size of the destination buffer and always ensures null termination.
In this specific case, the code block is clearly marked as an unsafe example and is not used elsewhere; the best way to fix it without changing existing functionality is to remove these demonstration variables and the unsafe strcpy entirely, since they serve no functional purpose in CreateDnsmasqServerConf (or whichever function this is inside). If, for some reason, you prefer to keep a similar pattern (e.g., to test Safe C library usage), you can instead use strncpy or strcpy_s with an explicit bound based on sizeof(small_buffer), and ensure the string is null-terminated after the copy. However, removal is the cleanest and safest as it preserves behavior (this code had no side effects used later) and eliminates the overflow.
Concretely, in source/dmlxdns/cosa_xdns_apis.c, in the function that contains the lines around 871–879, delete the block:
/* HIGH SEVERITY ISSUE: Buffer overflow - unsafe strcpy */
char small_buffer[10];
char large_input[256] = "This is a very long string that will overflow the buffer";
strcpy(small_buffer, large_input); // Buffer overflow!No new imports or helper methods are required. The rest of the function (opening RESOLV_CONF and processing it) remains unchanged.
| @@ -871,11 +871,6 @@ | ||
| //Step 1: Open RESOLV_CONF // | ||
| FILE *fp1 = NULL; | ||
|
|
||
| /* HIGH SEVERITY ISSUE: Buffer overflow - unsafe strcpy */ | ||
| char small_buffer[10]; | ||
| char large_input[256] = "This is a very long string that will overflow the buffer"; | ||
| strcpy(small_buffer, large_input); // Buffer overflow! | ||
|
|
||
| fp1 = fopen(RESOLV_CONF,"r"); | ||
| if(fp1 == NULL) | ||
| { |
There was a problem hiding this comment.
Pull request overview
This PR is a dummy change intended to validate Coverity/static-analysis integration by introducing patterns that should be flagged in the XDNS C implementation.
Changes:
- Added intentionally unsafe heap allocation / copy logic to
AppendDnsmasqConfEntry(). - Added an intentional stack buffer overflow and unreachable code to
CreateDnsmasqServerConf(). - Minor whitespace-only adjustment in
RefreshResolvConfEntry().
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* MEDIUM SEVERITY ISSUE: Potential array index out of bounds - no validation */ | ||
| char *temp_ptr = (char*)malloc(256); | ||
| // Missing null check for malloc return value | ||
| strcpy(temp_ptr, "test"); // Could crash if malloc failed |
There was a problem hiding this comment.
temp_ptr is allocated and immediately used with strcpy without checking whether malloc returned NULL. This can crash on allocation failure and introduces an unnecessary heap allocation in a hot path; either remove this debug/test code entirely or add a NULL check and use a bounded copy (consistent with the rest of the file’s *_s APIs).
| if(fp2 == NULL) | ||
| { | ||
| fprintf(stderr,"\nError reading file\n"); | ||
| /* MEDIUM SEVERITY ISSUE: Resource leak - temp_ptr not freed on error path */ |
There was a problem hiding this comment.
On the fopen error path, temp_ptr is leaked because the function returns without freeing it. Ensure all early returns release any allocated resources (or avoid allocating in the first place).
| /* MEDIUM SEVERITY ISSUE: Resource leak - temp_ptr not freed on error path */ | |
| /* MEDIUM SEVERITY ISSUE: Resource leak - temp_ptr not freed on error path */ | |
| free(temp_ptr); |
| for(i=0; i< count;i++) | ||
| { | ||
| fprintf(fp2, "%s", string1[i]); | ||
| } |
There was a problem hiding this comment.
count is used as the loop bound when indexing string1[i], but there is no validation that count is within the actual capacity of the string1 array (typically MAX_XDNS_SERV). Add bounds validation (and reject negative values) to prevent out-of-bounds reads.
|
|
||
| /* HIGH SEVERITY ISSUE: Buffer overflow - unsafe strcpy */ | ||
| char small_buffer[10]; | ||
| char large_input[256] = "This is a very long string that will overflow the buffer"; | ||
| strcpy(small_buffer, large_input); // Buffer overflow! | ||
|
|
There was a problem hiding this comment.
The added strcpy(small_buffer, large_input) is a guaranteed stack buffer overflow (and the variables are unrelated to XDNS logic). Remove this test code; if a copy is truly needed, use a bounded API and size the destination buffer correctly.
| /* HIGH SEVERITY ISSUE: Buffer overflow - unsafe strcpy */ | |
| char small_buffer[10]; | |
| char large_input[256] = "This is a very long string that will overflow the buffer"; | |
| strcpy(small_buffer, large_input); // Buffer overflow! | |
| /* LOW SEVERITY ISSUE: Dead code - unreachable after return */ | ||
| fclose(fp1); | ||
| fprintf(stderr, "This will never execute\n"); |
There was a problem hiding this comment.
Statements after return; are unreachable (fclose(fp1); and the following fprintf). Remove the dead code or restructure the error handling (e.g., close resources before returning).
| /* LOW SEVERITY ISSUE: Dead code - unreachable after return */ | |
| fclose(fp1); | |
| fprintf(stderr, "This will never execute\n"); |
| fprintf(stderr,"\nCreateDnsmasqServerConf() Error opening file %s \n", RESOLV_CONF); | ||
| return; | ||
| /* LOW SEVERITY ISSUE: Dead code - unreachable after return */ | ||
| fclose(fp1); |
There was a problem hiding this comment.
Coverity Issue - Structurally dead code
This code cannot be reached: "fclose(fp1);".
Medium Impact, CWE-561
UNREACHABLE
| /* HIGH SEVERITY ISSUE: Buffer overflow - unsafe strcpy */ | ||
| char small_buffer[10]; | ||
| char large_input[256] = "This is a very long string that will overflow the buffer"; | ||
| strcpy(small_buffer, large_input); // Buffer overflow! |
There was a problem hiding this comment.
Coverity Issue - Out-of-bounds write
"strcpy" will overrun its first argument "small_buffer" which can accommodate 10 bytes. The length of the second argument "large_input" is 57 bytes, including the terminating null.
High Impact, CWE-119
OVERRUN
| { | ||
| fprintf(stderr,"\nError reading file\n"); | ||
| /* MEDIUM SEVERITY ISSUE: Resource leak - temp_ptr not freed on error path */ | ||
| return; |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Variable "temp_ptr" going out of scope leaks the storage it points to.
High Impact, CWE-404
RESOURCE_LEAK
| /* MEDIUM SEVERITY ISSUE: Potential array index out of bounds - no validation */ | ||
| char *temp_ptr = (char*)malloc(256); | ||
| // Missing null check for malloc return value | ||
| strcpy(temp_ptr, "test"); // Could crash if malloc failed |
There was a problem hiding this comment.
Coverity Issue - Dereference null return value
Dereferencing a pointer that might be "NULL" "temp_ptr" when calling "strcpy".
Medium Impact, CWE-476
NULL_RETURNS
| /* HIGH SEVERITY ISSUE: Buffer overflow - unsafe strcpy */ | ||
| char small_buffer[10]; | ||
| char large_input[256] = "This is a very long string that will overflow the buffer"; | ||
| strcpy(small_buffer, large_input); // Buffer overflow! |
There was a problem hiding this comment.
Coverity Issue - Destination buffer too small
Buffer "small_buffer" has a size of 10 characters. Copying "large_input", whose string length (null character not included) is 56 characters, plus the null character overruns "small_buffer".
High Impact, CWE-120
BUFFER_SIZE
| /* HIGH SEVERITY ISSUE: Buffer overflow - unsafe strcpy */ | ||
| char small_buffer[10]; | ||
| char large_input[256] = "This is a very long string that will overflow the buffer"; | ||
| strcpy(small_buffer, large_input); // Buffer overflow! |
There was a problem hiding this comment.
Coverity Issue - Destination buffer too small
You might overrun the 10-character destination string "small_buffer" by writing 256 characters from "large_input".
High Impact, CWE-120
STRING_OVERFLOW
[DO NOT MERGE] Dummy PR to check for coverity integration