-
Notifications
You must be signed in to change notification settings - Fork 0
working server/client connection and copy file #4
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
base: master
Are you sure you want to change the base?
Changes from all commits
263968b
882f93c
6cad7f4
7f79e90
57ae222
3e93369
861ac7a
a88e197
f341c38
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,13 @@ | ||
| all: kilo | ||
| CC = gcc -g -Wall -W -ansi -pedantic -std=c99 -pthread -o | ||
| C+ = g++ -g -Wall -pthread -std=c++11 -o | ||
|
|
||
| all: kilo server | ||
|
|
||
| kilo: kilo.c | ||
| $(CC) -o kilo kilo.c -Wall -W -pedantic -std=c99 | ||
| $(CC) kilo kilo.c | ||
|
|
||
| server: server.cpp | ||
| $(C+) server server.cpp | ||
|
|
||
| clean: | ||
| rm kilo | ||
| rm -f kilo server transfer |
This file was deleted.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -53,6 +53,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #include <stdarg.h> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #include <fcntl.h> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #include <signal.h> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #include <netdb.h> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /* Syntax highlight types */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #define HL_NORMAL 0 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -109,6 +110,8 @@ struct editorConfig { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| struct editorSyntax *syntax; /* Current syntax highlight, or NULL. */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| //Note: we may want to add a few fields to the erow and editorConfig structs | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| static struct editorConfig E; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| enum KEY_ACTION{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -198,6 +201,7 @@ struct editorSyntax HLDB[] = { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #define HLDB_ENTRIES (sizeof(HLDB)/sizeof(HLDB[0])) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /* ======================= Low level terminal handling ====================== */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| //Note: probably don't need to edit these | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| static struct termios orig_termios; /* In order to restore at exit.*/ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -214,7 +218,7 @@ void editorAtExit(void) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| disableRawMode(STDIN_FILENO); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /* Raw mode: 1960 magic shit. */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /* Raw mode: 1960 magic*/ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| int enableRawMode(int fd) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| struct termios raw; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -362,6 +366,7 @@ int getWindowSize(int ifd, int ofd, int *rows, int *cols) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /* ====================== Syntax highlight color scheme ==================== */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| //Note: probably don't need to edit these | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| int is_separator(int c) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return c == '\0' || isspace(c) || strchr(",.()+-/*=~%[];",c) != NULL; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -551,6 +556,10 @@ void editorSelectSyntaxHighlight(char *filename) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /* ======================= Editor rows implementation ======================= */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| //Note: probably want to edit these. perhaps at the end of an update function, we call another | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // function to send an update message to the server. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| //Note: we can copy the logic from these functions to allow for editing after receuving an | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // update message from the server. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /* Update the rendered version and the syntax highlight of a row. */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| void editorUpdateRow(erow *row) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -616,7 +625,7 @@ void editorFreeRow(erow *row) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| free(row->hl); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /* Remove the row at the specified position, shifting the remainign on the | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /* Remove the row at the specified position, shifting the remaining on the | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * top. */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| void editorDelRow(int at) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| erow *row; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -852,6 +861,7 @@ int editorSave(void) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /* ============================= Terminal update ============================ */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| //Note: probably don't need to edit these | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /* We define a very simple "append buffer" structure, that is an heap | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * allocated string where we can append to. This is useful in order to | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -1008,6 +1018,7 @@ void editorSetStatusMessage(const char *fmt, ...) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /* =============================== Find mode ================================ */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| //Note: probably don't need to edit these | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #define KILO_QUERY_LEN 256 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -1107,6 +1118,7 @@ void editorFind(int fd) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /* ========================= Editor events handling ======================== */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| //Note: we probably don't need to edit these | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /* Handle cursor position change because arrow keys were pressed. */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| void editorMoveCursor(int key) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -1288,14 +1300,81 @@ void initEditor(void) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| signal(SIGWINCH, handleSigWinCh); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /* ========================= Communication with Server ======================== */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| void receiveFile(int fd){ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ssize_t n; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| FILE *file = fopen("transfer", "w"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| char buffer[1024]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| n = read(fd, buffer, 1024); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| buffer[n] = '\0'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1310
to
+1311
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Out-of-bounds write in At Was this helpful? React with 👍 or 👎 to provide feedback. Debug |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| while (1){ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if ((n = read(fd, buffer, 1024)) > 0){ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // printf("Line: %s\n", buffer); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| buffer[n] = '\0'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1310
to
+1316
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 In receiveFile() (kilo.c:1310-1316), buffer is declared as char[1024] but both read() calls pass 1024 as the count limit; when read() returns exactly 1024 bytes, buffer[1024] is written one past the end of the stack array (off-by-one stack overflow). Additionally, the initial read() at line 1310 has no error check: if read() returns -1 on a socket error, buffer[-1] is written, corrupting stack memory before the buffer. Fix: use read(fd, buffer, sizeof(buffer)-1) for both calls, and add if (n <= 0) return; before the first buffer[n] = '\0'. Extended reasoning...What the bug is and how it manifests receiveFile() in kilo.c declares a stack buffer of exactly 1024 bytes: The specific code paths that trigger it Issue 1 (overflow): The server explicitly calls Issue 2 (underflow): The initial read at line 1310 has no error guard. If the socket experiences an error (connection reset, network failure, etc.), read() returns -1. Why existing code doesn't prevent it There is no bounds check between the read() call and the buffer[n] write. C does not prevent out-of-bounds writes. The only guard in the function is the What the impact would be Both issues constitute undefined behavior with concrete consequences. The off-by-one overflow (n=1024) overwrites the byte immediately following buffer on the stack — potentially part of the saved frame pointer, return address, or adjacent local variables, enabling stack corruption and possible code execution. The underflow (n=-1) writes to How to fix it Change both read() calls to request at most Step-by-step proof
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!strcmp(buffer, "End Transfer")){ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // printf("Closing\n"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fclose(file); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fprintf(file, "%s\n", buffer); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| send(fd, "ACK", 1024, 0); //why are we sending this? | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 send() reads 1024 bytes from a 4-byte string literal "ACK", causing buffer over-read
Suggested change
Was this helpful? React with 👍 or 👎 to provide feedback. DebugThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 All four protocol message sends pass 1024 as the byte count but use short string literals ("ACK", "get", "Start Transfer", "End Transfer"), causing send() to read 1021–1021 bytes past the string into adjacent .rodata memory. Fix by using strlen() or the exact literal length for each call: send(fd, "ACK", 3, 0), send(serverFd, "get", 3, 0), send(fd, "Start Transfer", 14, 0), and send(fd, "End Transfer", 12, 0). Extended reasoning...What the bug is and how it manifests The code passes the buffer size constant (1024) as the third argument to send() when sending short string literals. For example, The specific code paths that trigger it There are four affected call sites:
Why existing code doesn't prevent it The C standard does not bound-check pointer arithmetic or memory reads that stay within mapped pages. String literals in C reside in the .rodata segment, which is a readable (though non-writable) mapping. Adjacent constants, function pointers encoded in the binary, version strings, and other read-only data from the same compilation unit typically follow in the same page, so there is no segfault — the extra bytes are silently read and transmitted. What the impact would be This is an information disclosure vulnerability. Every "ACK" acknowledgment sends ~1020 bytes of process memory contents (other string literals, constants, possibly embedded paths or other sensitive data compiled into the binary) to the remote peer. While the server currently reads and discards the ACK bytes, the data is still transmitted on the wire. For "get" in main(), the server receives 1021 extra bytes which it compares against the string "get" using a std::string equality check — the comparison happens to work because std::string uses the length it received, not a fixed-size comparison, but the extra garbage bytes still traverse the network. How to fix it Replace the hardcoded 1024 with the actual string length at each call site:
Step-by-step proof
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1313
to
+1325
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 The Extended reasoning...What the bug is and how it manifests The loop in while (1) {
if ((n = read(fd, buffer, 1024)) > 0) {
buffer[n] = '\0';
if (\!strcmp(buffer, "End Transfer")) {
fclose(file);
return;
}
fprintf(file, "%s\n", buffer);
send(fd, "ACK", 1024, 0);
}
}When the server closes the TCP connection, The specific code path that triggers it The only exit path from the function is Why existing code doesn't prevent it The programmer placed the What the impact would be Any abnormal server termination (crash, kill signal, network drop) permanently hangs the kilo client process, consuming 100% CPU indefinitely. The client must be killed manually with SIGKILL. The open file handle to "transfer" is also leaked since Step-by-step proof
How to fix it Change the loop from: while (1) {
if ((n = read(fd, buffer, 1024)) > 0) { ... }
}to: while ((n = read(fd, buffer, 1023)) > 0) {
buffer[n] = '\0';
...
}
fclose(file);This exits the loop on both EOF (n=0) and error (n=-1), and ensures |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| //editorOpen("test"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1305
to
+1327
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Multiple critical issues in
🐛 Proposed fix void receiveFile(int fd){
ssize_t n;
FILE *file = fopen("transfer", "w");
+ if (!file) {
+ perror("Cannot create transfer file");
+ return;
+ }
char buffer[1024];
n = read(fd, buffer, 1024);
- buffer[n] = '\0';
+ if (n <= 0) {
+ fclose(file);
+ return;
+ }
+ buffer[n < 1024 ? n : 1023] = '\0';
- while (1){
- if ((n = read(fd, buffer, 1024)) > 0){
+ while ((n = read(fd, buffer, 1023)) > 0){
buffer[n] = '\0';
if (!strcmp(buffer, "End Transfer")){
fclose(file);
return;
}
fprintf(file, "%s\n", buffer);
- send(fd, "ACK", 1024, 0);
- }
+ send(fd, "ACK", 3, 0);
}
+ fclose(file);
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /* ============================= Main Program ================================== */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| //main program of text-editor client | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| int main(int argc, char **argv) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (argc != 2) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fprintf(stderr,"Usage: kilo <filename>\n"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| //check command-line args | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (argc != 3) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fprintf(stderr,"Usage: kilo <host> <port>\n"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| exit(1); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| initEditor(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| editorSelectSyntaxHighlight(argv[1]); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| //setup | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| struct addrinfo hints, *res, *traverser; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| memset(&hints, 0, sizeof(struct addrinfo)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| hints.ai_family = AF_UNSPEC; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| hints.ai_socktype = SOCK_STREAM; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| int r = getaddrinfo(argv[1], argv[2], &hints, &res); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (r != 0){ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fprintf(stderr,"Error: Can't find server.\n"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return 1; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Try addresses until one is successful | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| int serverFd; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for (traverser = res; traverser; traverser = traverser->ai_next){ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if ((serverFd = socket(traverser->ai_family, traverser->ai_socktype, traverser->ai_protocol)) != -1){ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if ((connect(serverFd, traverser->ai_addr, traverser->ai_addrlen)) == 0){ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| break; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| printf("Connected\n"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1358
to
+1359
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 No check for connection failure after address traversal loop After the
Suggested change
Was this helpful? React with 👍 or 👎 to provide feedback. Debug
Comment on lines
+1350
to
+1359
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Connection loop doesn't verify success and leaks If no address succeeds, 🐛 Proposed fix for (traverser = res; traverser; traverser = traverser->ai_next){
if ((serverFd = socket(traverser->ai_family, traverser->ai_socktype, traverser->ai_protocol)) != -1){
if ((connect(serverFd, traverser->ai_addr, traverser->ai_addrlen)) == 0){
break;
}
+ close(serverFd);
}
}
+ freeaddrinfo(res);
+ if (!traverser) {
+ fprintf(stderr, "Error: Could not connect to server.\n");
+ return 1;
+ }
printf("Connected\n");📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1352
to
+1360
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 The connection loop at kilo.c lines 1352-1360 has two bugs: (1) if all socket()/connect() attempts fail, traverser ends up NULL but the code unconditionally prints "Connected" and passes the invalid serverFd to send() and receiveFile(), causing undefined behavior; (2) freeaddrinfo(res) is never called on any code path, leaking the entire addrinfo linked list returned by getaddrinfo(). Fix by adding a traverser == NULL check with early return after the loop, and calling freeaddrinfo(res) before any return path. Extended reasoning...What the bug is and how it manifests The networking code in main() calls getaddrinfo() to resolve the server hostname, then iterates over the returned linked list trying socket() and connect() on each candidate address. If every candidate fails (socket() returns -1 or connect() returns non-zero), the loop exits with traverser == NULL and serverFd holding either -1 (from a failed socket()) or a file descriptor for a socket that failed to connect. The code then unconditionally executes printf("Connected\n") and falls through to the user input loop, which calls send(serverFd, "get", 1024, 0) and receiveFile(serverFd) using this invalid fd. Separately, freeaddrinfo(res) is never called on any code path — not on success, not on failure, and not before process exit — leaking all memory allocated by getaddrinfo. The specific code path that triggers it At line 1344, getaddrinfo() allocates a linked list rooted at res. The for-loop at lines 1352-1358 iterates traverser over this list; on break (success), traverser points to the winning addrinfo node. If the loop completes without break, traverser == NULL. Line 1359 then executes printf("Connected\n") unconditionally — no check on traverser. Lines 1366-1370 then call send(serverFd, ...) and receiveFile(serverFd) with whatever serverFd happens to hold. freeaddrinfo(res) appears nowhere in the file. Why existing code doesn't prevent it There is simply no if (!traverser) guard between the end of the for-loop and the printf. The pattern of iterating an addrinfo list with a break-on-success requires an explicit post-loop check to distinguish the success case (traverser != NULL) from the exhausted-all-addresses case (traverser == NULL). The current code omits this check entirely. The freeaddrinfo leak is equally straightforward: there is no call site for it in the file at all. What the impact would be If the server is unreachable (wrong hostname, server down, firewall blocking the port), every address attempt fails, traverser is NULL, and the program silently reports "Connected" — a false positive that hides the real error. Subsequent send() on the invalid fd returns -1 with EBADF or ENOTCONN (return value not checked), and receiveFile() calls read() on the same bad fd — also EBADF — leading to undefined behavior in the buffer indexing (buffer[n] where n == -1 writes buffer[-1]). The memory leak from freeaddrinfo is minor relative to the functional failure but still a resource bug on every invocation. How to fix it After the for-loop, call freeaddrinfo(res) unconditionally, then check traverser: if (!traverser) { fprintf(stderr, "Error: Could not connect\n"); return 1; }. Additionally, close(serverFd) should be called inside the loop when connect() fails to avoid leaking socket file descriptors during the address traversal. Step-by-step proof of the traverser NULL path
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| char buffer[1024]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| while (fgets(buffer, 1024, stdin)){ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| buffer[strcspn(buffer, "\r\n")] = 0; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!strcmp(buffer, "get")){ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| printf("sending get\n"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| send(serverFd, "get", 1024, 0); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 send() reads 1024 bytes from a 4-byte string literal "get", causing buffer over-read
Suggested change
Was this helpful? React with 👍 or 👎 to provide feedback. Debug |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| receiveFile(serverFd); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // printf("%s\n", buffer); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1361
to
+1370
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
🐛 Proposed fix-send(serverFd, "get", 1024, 0);
+send(serverFd, "get", 3, 0);📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| close(serverFd); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // exit(0); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| //start editor | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| initEditor(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| editorSelectSyntaxHighlight("transfer"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| editorOpen(argv[1]); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 editorOpen opens hostname (argv[1]) instead of the received "transfer" file
Suggested change
Was this helpful? React with 👍 or 👎 to provide feedback. Debug
Comment on lines
+1375
to
1378
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
After the networking changes, 🐛 Proposed fix initEditor();
editorSelectSyntaxHighlight("transfer");
- editorOpen(argv[1]);
+ editorOpen("transfer");📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
Comment on lines
+1377
to
1378
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 After receiving the file via the server and storing it as "transfer", the code calls editorOpen(argv[1]) where argv[1] is the server hostname (e.g. "localhost"), not the received file. This means the editor will attempt to open a file named after the hostname instead of the transferred content; the fix is to change editorOpen(argv[1]) to editorOpen("transfer"). Extended reasoning...What the bug is and how it manifests In kilo.c's The specific code path that triggers it kilo.c lines 1377–1378: editorSelectSyntaxHighlight("transfer");
editorOpen(argv[1]); // argv[1] == hostname, not "transfer"Both calls are in the same block right after Why existing code does not prevent it
What the impact would be Users who run Step-by-step proof
How to fix it Change |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| enableRawMode(STDIN_FILENO); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| editorSetStatusMessage( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1362
to
1380
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚩 Editor code is unreachable after stdin EOF in client main() The (Refers to lines 1362-1385) Was this helpful? React with 👍 or 👎 to provide feedback. Debug
Comment on lines
+1376
to
1380
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Editor code unreachable — stdin is at EOF after fgets loop completes At (Refers to lines 1376-1384) Prompt for agentsWas this helpful? React with 👍 or 👎 to provide feedback. Debug |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,143 @@ | ||||||||||||||||||||||
| // C Headers | ||||||||||||||||||||||
| #include <arpa/inet.h> | ||||||||||||||||||||||
| #include <errno.h> | ||||||||||||||||||||||
| #include <netdb.h> | ||||||||||||||||||||||
| #include <netinet/in.h> | ||||||||||||||||||||||
| #include <pthread.h> | ||||||||||||||||||||||
| #include <signal.h> | ||||||||||||||||||||||
| #include <stdlib.h> | ||||||||||||||||||||||
| #include <string.h> | ||||||||||||||||||||||
| #include <sys/socket.h> | ||||||||||||||||||||||
| #include <sys/types.h> | ||||||||||||||||||||||
| #include <unistd.h> | ||||||||||||||||||||||
| // C++ Headers | ||||||||||||||||||||||
| #include <fstream> | ||||||||||||||||||||||
| #include <iostream> | ||||||||||||||||||||||
| #include <sstream> | ||||||||||||||||||||||
| #include <map> | ||||||||||||||||||||||
| #include <string> | ||||||||||||||||||||||
| #include <vector> | ||||||||||||||||||||||
| using namespace std; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| //readFile - reads lines in file into the data structure lines | ||||||||||||||||||||||
| vector<string> readFile(){ | ||||||||||||||||||||||
| string line; | ||||||||||||||||||||||
| vector<string> lines; | ||||||||||||||||||||||
| ifstream file("test"); | ||||||||||||||||||||||
| while (getline(file, line)){ | ||||||||||||||||||||||
| lines.push_back(line); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| return lines; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| //sendFile - send file line-by-line to a client at fd | ||||||||||||||||||||||
| void sendFile(int fd, vector<string> lines){ | ||||||||||||||||||||||
| vector<string>::iterator i; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| send(fd, (void*)"Start Transfer", 1024, 0); | ||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 send() reads 1024 bytes from short string literals in sendFile, causing buffer over-read
Suggested change
Was this helpful? React with 👍 or 👎 to provide feedback. Debug |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /* for (i = lines.begin(); i != lines.end(); i++){ | ||||||||||||||||||||||
| char buffer[1024]; | ||||||||||||||||||||||
| string msg = *i; | ||||||||||||||||||||||
| // cout << "Line: " << msg << endl; | ||||||||||||||||||||||
| send(fd, msg.c_str(), msg.length(), 0); | ||||||||||||||||||||||
| read(fd, buffer, 1024); | ||||||||||||||||||||||
| } */ | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| //this is the same as the loop that is commented out | ||||||||||||||||||||||
| for(string msg : lines){ | ||||||||||||||||||||||
| char buffer[1024]; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| send(fd, msg.c_str(), msg.length(), 0); | ||||||||||||||||||||||
| read(fd, buffer, 1024); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| send(fd, (void*)"End Transfer", 1024, 0); | ||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 send() reads 1024 bytes from 13-byte "End Transfer" string literal Same buffer over-read as
Suggested change
Was this helpful? React with 👍 or 👎 to provide feedback. Debug
Comment on lines
+48
to
+55
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚩 Protocol mismatch: server sends msg.length() bytes but client expects null-terminated strings In Was this helpful? React with 👍 or 👎 to provide feedback. Debug |
||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
Comment on lines
+34
to
+56
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Protocol deadlock: ACK synchronization mismatch with client. The server sends "Start Transfer" (Line 37) and immediately proceeds to send file lines, expecting an ACK after each (Line 52). However, the client's Additionally, 🐛 Proposed fix for send length issue-send(fd, (void*)"Start Transfer", 1024, 0);
+send(fd, "Start Transfer", strlen("Start Transfer"), 0);
// ...
-send(fd, (void*)"End Transfer", 1024, 0);
+send(fd, "End Transfer", strlen("End Transfer"), 0);The ACK protocol needs to be redesigned so both sides agree on when ACKs are sent. Either:
🤖 Prompt for AI Agents |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| //threadFunc - thread function to read any messages from a client | ||||||||||||||||||||||
| void *threadFunc(void *args){ | ||||||||||||||||||||||
| ssize_t n; | ||||||||||||||||||||||
| int clientFd = *(int*)args; | ||||||||||||||||||||||
| char buffer[1024]; | ||||||||||||||||||||||
| pthread_detach(pthread_self()); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Read until disconnection | ||||||||||||||||||||||
| while ((n = read(clientFd, buffer, 1024)) > 0){ | ||||||||||||||||||||||
| string line(buffer); | ||||||||||||||||||||||
|
Comment on lines
+66
to
+67
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Buffer not null-terminated after read() in threadFunc, causing undefined string construction
Suggested change
Was this helpful? React with 👍 or 👎 to provide feedback. Debug
Comment on lines
+66
to
+67
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 In server.cpp threadFunc (lines 66-67), read(clientFd, buffer, 1024) fills a 1024-byte stack buffer but string line(buffer) is constructed immediately without null-terminating first; if exactly 1024 bytes are received, the std::string const char* constructor reads past buffer[1023] into unowned memory looking for a null terminator, causing undefined behavior. Fix by using read(clientFd, buffer, 1023) followed by buffer[n] = '\0', or by constructing via std::string(buffer, n). Extended reasoning...What the bug is and how it manifests In threadFunc (server.cpp line 61), buffer is declared as char buffer[1024], giving valid indices 0–1023. Line 66 reads from the client socket with read(clientFd, buffer, 1024), which can return up to 1024 bytes. Line 67 then constructs std::string line(buffer), which invokes the const char* constructor. This constructor scans forward from the pointer looking for a null byte to determine the string's length. If read() returns exactly 1024, all 1024 bytes of buffer are filled with data and no null byte exists within the array; the constructor continues scanning past buffer[1023] into adjacent stack memory (undefined behavior). The specific code path that triggers it The client in kilo.c sends commands via send(serverFd, "get", 1024, 0), which sends a full 1024 bytes. When this 1024-byte payload arrives at the server in a single read() call, n == 1024 and buffer[0]..buffer[1023] are all occupied by data from the network. The string line(buffer) call on the very next line then reads past the array boundary searching for '\0'. Why existing code doesn't prevent it There is no null-termination step between read() and the string constructor. Unlike the inner loop in kilo.c's receiveFile() (which does buffer[n] = '\0'), threadFunc has no such guard. The while loop condition only checks n > 0, which is satisfied when n == 1024, so no early exit occurs. What the impact would be When n == 1024, the string constructor performs an out-of-bounds read on the stack, which is undefined behavior under the C++ standard. In practice this corrupts the string's length and content, causing the subsequent comparisons line == "exit" and line == "get" to fail even when the intended command was sent, breaking the server's command dispatch entirely. Depending on stack layout, it can also trigger crashes or security vulnerabilities. How to fix it Option 1: Limit read to 1023 bytes and null-terminate: change line 66 to while ((n = read(clientFd, buffer, 1023)) > 0) and add buffer[n] = '\0' before the string construction on line 67. Option 2: Use the length-aware constructor: std::string line(buffer, n), which does not require a null terminator and constructs exactly n bytes. Step-by-step proof
🔬 also observed by staging-devin-ai-integration |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if (line == "exit"){ | ||||||||||||||||||||||
| close(clientFd); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
Comment on lines
+66
to
+71
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Buffer not null-terminated before string construction.
🐛 Proposed fix while ((n = read(clientFd, buffer, 1024)) > 0){
+ buffer[n] = '\0';
string line(buffer);
- if (line == "exit"){
+ if (line == "exit" || line == "exit\n"){
close(clientFd);
+ break;
}🤖 Prompt for AI Agents |
||||||||||||||||||||||
| else if (line == "get"){ | ||||||||||||||||||||||
| cout << "Get Received" << endl; | ||||||||||||||||||||||
| sendFile(clientFd, readFile()); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| return NULL; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
Comment on lines
+66
to
+78
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚩 Server thread doesn't close clientFd on normal read loop exit In Was this helpful? React with 👍 or 👎 to provide feedback. Debug |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| //handleSigInt - action performed when user types ctrl-C | ||||||||||||||||||||||
| void handleSigInt(int unused __attribute__((unused))){ | ||||||||||||||||||||||
| exit(0); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| //main server program | ||||||||||||||||||||||
| int main(void){ | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| //setup SIGINT signal handler | ||||||||||||||||||||||
| signal(SIGINT, handleSigInt); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Create server socket | ||||||||||||||||||||||
| int serverFd; | ||||||||||||||||||||||
| if ((serverFd = socket(AF_INET, SOCK_STREAM, 0)) < 0){ | ||||||||||||||||||||||
| std::cerr << "Error: Can't create socket." << std::endl; | ||||||||||||||||||||||
| return 1; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Set options for socket | ||||||||||||||||||||||
| int val; | ||||||||||||||||||||||
| if (setsockopt(serverFd, SOL_SOCKET, SO_REUSEADDR, &val, sizeof(int))){ | ||||||||||||||||||||||
|
Comment on lines
+99
to
+100
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Uninitialized value passed to setsockopt for SO_REUSEADDR
Suggested change
Was this helpful? React with 👍 or 👎 to provide feedback. Debug |
||||||||||||||||||||||
| std::cerr << "Error: Can't reuse socket." << std::endl; | ||||||||||||||||||||||
| return 2; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
Comment on lines
+99
to
+103
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Uninitialized variable passed to
🐛 Proposed fix // Set options for socket
-int val;
+int val = 1;
if (setsockopt(serverFd, SOL_SOCKET, SO_REUSEADDR, &val, sizeof(int))){📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
Comment on lines
+99
to
+103
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 In server.cpp line 99, Extended reasoning...What the bug is and how it manifests At server.cpp line 99, the code declares The specific code path that triggers it The sequence is: socket() creates serverFd at ~line 95, then lines 99-103: int val; // uninitialized
if (setsockopt(serverFd, SOL_SOCKET, SO_REUSEADDR, &val, sizeof(int))){
std::cerr << "Error: Can't reuse socket." << std::endl;
return 2;
}This is in main(), which runs on a fresh stack frame. The local variable val is never assigned before its address is passed to setsockopt. Why existing code doesn't prevent it There is no initializer on the declaration and no assignment before the setsockopt call. The error check only validates that setsockopt itself didn't fail (return non-zero errno), but setsockopt with a zero value succeeds without error — it simply disables the option rather than enabling it. So the error check provides no protection against the wrong value being passed. What the impact would be SO_REUSEADDR is intended to allow the server to bind to a port that's still in TIME_WAIT state after a previous run. Without it enabled, restarting the server quickly after stopping it will fail with "Address already in use" (EADDRINUSE) until the OS releases the port (typically 60–120 seconds). On most platforms, freshly allocated stack frames in main() are zeroed by the OS when a new process starts (stack pages come from zero-filled physical pages), meaning val will frequently be 0, disabling SO_REUSEADDR in practice. Additionally, reading an uninitialized variable is formally undefined behavior in C++, which compilers are permitted to optimize in unexpected ways. How to fix it Change line 99 from Step-by-step proof
🔬 also observed by coderabbitai |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Configure addr and bind | ||||||||||||||||||||||
| struct sockaddr_in serverAddr; | ||||||||||||||||||||||
| serverAddr.sin_family = AF_INET; | ||||||||||||||||||||||
| serverAddr.sin_addr.s_addr = htonl(INADDR_ANY); | ||||||||||||||||||||||
| serverAddr.sin_port = htons(10001); | ||||||||||||||||||||||
| if (bind(serverFd, (struct sockaddr *)&serverAddr, sizeof(serverAddr)) == -1){ | ||||||||||||||||||||||
| cerr << "Error: Can't bind socket to port." << endl; | ||||||||||||||||||||||
| return 3; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Listen for client connections | ||||||||||||||||||||||
| if (listen(serverFd, 20) < 0){ | ||||||||||||||||||||||
| cerr << "Error: Can't listen for clients." << endl; | ||||||||||||||||||||||
| return 4; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Get name and port assigned to server | ||||||||||||||||||||||
| char *name = new char[1024]; | ||||||||||||||||||||||
| struct sockaddr_in infoAddr; | ||||||||||||||||||||||
| socklen_t len = sizeof(infoAddr); | ||||||||||||||||||||||
| gethostname(name, 1024); | ||||||||||||||||||||||
| getsockname(serverFd, (struct sockaddr *)&infoAddr, &len); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Report name and port | ||||||||||||||||||||||
| cout << name << ":" << ntohs(serverAddr.sin_port)<< endl; | ||||||||||||||||||||||
| delete name; | ||||||||||||||||||||||
|
Comment on lines
+122
to
+130
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Memory allocation/deallocation mismatch and missing error check.
🐛 Proposed fix // Get name and port assigned to server
-char *name = new char[1024];
+char name[1024];
struct sockaddr_in infoAddr;
socklen_t len = sizeof(infoAddr);
-gethostname(name, 1024);
+if (gethostname(name, 1024) == -1) {
+ cerr << "Error: Can't get hostname." << endl;
+}
getsockname(serverFd, (struct sockaddr *)&infoAddr, &len);
// Report name and port
cout << name << ":" << ntohs(serverAddr.sin_port)<< endl;
-delete name;🧰 Tools🪛 Cppcheck (2.20.0)[error] 130-130: Mismatching allocation and deallocation (mismatchAllocDealloc) 🤖 Prompt for AI AgentsThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 delete instead of delete[] for array allocation
Suggested change
Was this helpful? React with 👍 or 👎 to provide feedback. Debug
Comment on lines
+122
to
+130
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 In server.cpp at line 122, name is allocated with new char[1024] (array form) but freed at line 130 with delete name (scalar form); the C++ standard requires delete[] for array allocations. Using scalar delete on an array-allocated pointer is undefined behavior that may corrupt heap allocator internal state. Fix: change delete name to delete[] name, or use a stack-allocated char name[1024] and remove the delete entirely. Extended reasoning...What the bug is and how it manifests At server.cpp line 122, the code allocates a hostname buffer as an array: char *name = new char[1024]. This is the array form of new, which allocates 1024 chars and requires a matching delete[] to free them. However, at line 130, the code frees the buffer with delete name — the scalar form of delete. This mismatch is explicitly undefined behavior per the C++ standard (ISO/IEC 14882, [expr.delete]). The specific code path that triggers it The code path is straightforward and executes on every server startup, unconditionally. After binding and listening, the server calls gethostname(name, 1024) and getsockname(...) to report the server address, then immediately frees the buffer with delete name before entering the accept() loop. This sequence always runs. Why existing code doesn't prevent it There is no mechanism in C++ that prevents a scalar delete from being called on an array-allocated pointer at the language level. The compiler may warn (some do), but the code compiles and links. The runtime has no guard: delete calls the global operator delete, which does not know whether the pointer originated from new or new[]. For types with trivial destructors (like char), the observable behavior on common implementations is often identical, but the C++ standard explicitly makes this undefined behavior regardless. What the impact would be On implementations where new[] stores a count before the allocation (a common implementation detail for tracking how many destructors to call on delete[]), scalar delete passes the raw pointer to the allocator, which may then misinterpret allocation metadata — potentially corrupting the heap's free-list bookkeeping. While char has a trivial destructor (so no destructor miscounting occurs), the pointer passed to the allocator's dealloc function may differ by sizeof(size_t) bytes from what the allocator expects, depending on the ABI. This can cause silent heap corruption or a crash in subsequent allocations. Static analyzers including Cppcheck flag this as a definite error (mismatchAllocDealloc). How to fix it Two options: (1) Change line 130 from delete name to delete[] name — the minimal correct fix. (2) Replace the heap allocation entirely with a stack array: char name[1024]; and remove the delete line — this is simpler and avoids the issue altogether. Since name is a fixed-size scratch buffer that never escapes the function, a stack allocation is appropriate and preferred. Step-by-step proof
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Connect a client | ||||||||||||||||||||||
| struct sockaddr_in cliAddr; | ||||||||||||||||||||||
| len = sizeof(cliAddr); | ||||||||||||||||||||||
| while (true){ | ||||||||||||||||||||||
| int clientFd = accept(serverFd, (struct sockaddr *)&serverAddr, &len); | ||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 accept() overwrites serverAddr instead of populating cliAddr
Suggested change
Was this helpful? React with 👍 or 👎 to provide feedback. Debug |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Create thread to deal with client | ||||||||||||||||||||||
| pthread_t thread; | ||||||||||||||||||||||
| pthread_create(&thread, NULL, threadFunc, (void *)&clientFd); | ||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Race condition: address of loop-local variable passed to thread
Was this helpful? React with 👍 or 👎 to provide feedback. Debug |
||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
Comment on lines
+135
to
+141
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Race condition:
🐛 Proposed fix while (true){
- int clientFd = accept(serverFd, (struct sockaddr *)&serverAddr, &len);
+ int *clientFd = new int;
+ *clientFd = accept(serverFd, (struct sockaddr *)&cliAddr, &len);
+ if (*clientFd < 0) {
+ delete clientFd;
+ continue;
+ }
// Create thread to deal with client
pthread_t thread;
- pthread_create(&thread, NULL, threadFunc, (void *)&clientFd);
+ pthread_create(&thread, NULL, threadFunc, (void *)clientFd);
}Then in void *threadFunc(void *args){
int clientFd = *(int*)args;
delete (int*)args; // Free the allocated int
// ... rest of functionAlso note: Line 136 passes 🤖 Prompt for AI Agents
Comment on lines
+135
to
+141
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Two bugs in the accept loop in server.cpp lines 135–141: (1) accept() passes &serverAddr instead of &cliAddr, overwriting the server's own address struct on every incoming connection — cliAddr is declared at line 133 specifically for this purpose but is never used; (2) pthread_create receives &clientFd where clientFd is a stack-local loop variable, so the main thread can overwrite it before the spawned thread dereferences it at line 61, causing threads to operate on the wrong file descriptor. Fix by passing &cliAddr to accept(), and heap-allocating a new int(clientFd) per connection to pass to pthread_create, freeing it inside threadFunc after reading. Extended reasoning...Bug 1 — accept() passes &serverAddr instead of &cliAddr The accept call on line 136 is written as: Bug 2 — Race condition: loop-local clientFd stack address passed to pthread_create On line 140, pthread_create receives (void *)&clientFd where clientFd is declared inside the while(true) body on line 136. In C++, a variable declared inside a loop body occupies a fixed stack slot that is reused on every iteration. The main thread calls pthread_create and then immediately loops back to the top of the while(true), where accept() assigns a new value to the same stack address (clientFd). Meanwhile, the spawned thread has not necessarily yet executed line 61: int clientFd = (int)args. If the main thread reaches accept() — and especially if another connection arrives — before the new thread reads the pointed-to int, the thread reads the wrong file descriptor (the one from the next accept() call). Multiple threads can end up sharing a single fd, and the fd intended for one of them is leaked with no close(). Why existing code does not prevent the race There is no synchronization between pthread_create returning and the new thread reading its argument. pthread_create only guarantees the thread will eventually start; it places no ordering constraint on when the thread first executes. On a busy server or a system where the scheduler defers the new thread, the main loop is free to overrun clientFd before the thread reads it. This is a textbook TOCTOU race described in every pthreads tutorial. Impact Under concurrent load (two clients connecting in rapid succession), the second client's fd may be read by the first thread, causing both threads to interact with the same client while the other client is abandoned. Depending on timing, this also causes double-close of a file descriptor — undefined behavior that can silently corrupt subsequent open() or accept() calls that reuse the same fd number. How to fix both bugs
Step-by-step proof of the race
|
||||||||||||||||||||||
| return 0; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| this is a test file | ||
| these are some words | ||
| these are some more | ||
| bye! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 In receiveFile() at kilo.c:1307, fopen("transfer", "w") is called but the return value is never checked for NULL. If the file cannot be created (e.g., permission denied, disk full, read-only filesystem), the subsequent fprintf(file, ...) and fclose(file) calls will dereference the NULL pointer, causing a segfault. Fix by adding if (!file) { perror("fopen"); return; } immediately after the fopen call.
Extended reasoning...
What the bug is and how it manifests
In receiveFile() (kilo.c:1307), the code calls FILE file = fopen("transfer", "w") to open the local output file for writing. The return value of fopen() is stored in file but is never checked before use. If fopen() fails for any reason, it returns NULL, and the code continues to use this NULL pointer as a valid FILE.
The specific code path that triggers it
The call at line 1307 is: FILE *file = fopen("transfer", "w"). After that, the function directly proceeds to use file in the inner loop at line 1322: fprintf(file, "%s\n", buffer) — and in the cleanup at line 1319: fclose(file). Neither usage has any guard against file being NULL.
Why existing code doesn't prevent it
There is no null check anywhere between the fopen() call and the first use of file. The function does not validate the file pointer before passing it to fprintf() or fclose(). The C standard library does not protect against NULL FILE* dereferences; passing NULL to fprintf() or fclose() is undefined behavior that typically results in a segfault.
What the impact would be
If kilo is run in a directory where "transfer" cannot be created (e.g., a read-only filesystem, no write permission, or disk full), the process will segfault immediately upon the first get command. This is a hard crash with no error message to the user, making diagnosis difficult. In a security context, a remote server could potentially trigger this crash by causing the client to attempt a file transfer in adverse filesystem conditions.
How to fix it
Add an early-exit guard immediately after the fopen() call:
Step-by-step proof