Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions Makefile
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
5 changes: 4 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,11 @@ Kilo is a small text editor in less than 1K lines of code (counted with cloc).
A screencast is available here: https://asciinema.org/a/90r2i9bq8po03nazhqtsifksb

Usage: kilo `<filename>`
New Usage: kilo <host> <port>

Keys:
'get' to copy file from server
Comment on lines 8 to +11

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Clarify or remove the deprecated usage line.

Line 8 shows kilo <filename> which is no longer valid based on the code changes in main(). Either remove it or clearly mark it as deprecated. Also, the 'get' instruction is vague—users need to know:

  • How to start the server first
  • That they type get at the stdin prompt after connecting
  • That the file is saved as transfer locally
📝 Suggested documentation improvement
-Usage: kilo `<filename>`
-New Usage: kilo <host> <port>
+Usage: kilo <host> <port>
 
-'get' to copy file from server
+Server Setup:
+    1. Build with `make`
+    2. Run `./server` (listens on port 10001)
+
+Client Usage:
+    1. Run `./kilo <host> <port>` (e.g., `./kilo localhost 10001`)
+    2. Type `get` and press Enter to download the file
+    3. Press Ctrl+D to exit the command loop and open the editor
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Usage: kilo `<filename>`
New Usage: kilo <host> <port>
Keys:
'get' to copy file from server
Usage: kilo <host> <port>
Server Setup:
1. Build with `make`
2. Run `./server` (listens on port 10001)
Client Usage:
1. Run `./kilo <host> <port>` (e.g., `./kilo localhost 10001`)
2. Type `get` and press Enter to download the file
3. Press Ctrl+D to exit the command loop and open the editor
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@README.md` around lines 8 - 11, Update README to remove or mark the old "kilo
<filename>" usage as deprecated and replace it with the current invocation "kilo
<host> <port>" (reflecting the main() change), and expand the 'get' instructions
to explicitly state how to start the server first, that the client connects
using "kilo <host> <port>", that the user types get at the stdin prompt after
connecting, and that the received file is written locally as "transfer";
reference the CLI entrypoint "main()" and the command keyword "get" in the doc
text so readers can correlate behavior to the code.


Editor Keys:

CTRL-S: Save
CTRL-Q: Quit
Expand Down
10 changes: 0 additions & 10 deletions TODO

This file was deleted.

91 changes: 85 additions & 6 deletions kilo.c
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
#include <stdarg.h>
#include <fcntl.h>
#include <signal.h>
#include <netdb.h>

/* Syntax highlight types */
#define HL_NORMAL 0
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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.*/

Expand All @@ -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;

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -1008,6 +1018,7 @@ void editorSetStatusMessage(const char *fmt, ...) {
}

/* =============================== Find mode ================================ */
//Note: probably don't need to edit these

#define KILO_QUERY_LEN 256

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Buffer overflow: buffer[n] writes out-of-bounds when read() returns 1024

buffer is declared as char buffer[1024] (valid indices 0–1023). When read(fd, buffer, 1024) returns 1024, buffer[n] = '\0' writes to buffer[1024], one byte past the array. Additionally, if read() returns -1 (error), buffer[-1] is accessed. Both are undefined behavior.

Affected lines

Line 1310-1311 (first read):

n = read(fd, buffer, 1024);
buffer[n] = '\0';

Line 1314-1316 (loop read):

if ((n = read(fd, buffer, 1024)) > 0){
    buffer[n] = '\0';
Suggested change
n = read(fd, buffer, 1024);
buffer[n] = '\0';
n = read(fd, buffer, 1023);
if (n < 0) n = 0;
buffer[n] = '\0';
Staging: Open in Devin

Was this helpful? React with 👍 or 👎 to provide feedback.

Debug

Playground


while (1){
if ((n = read(fd, buffer, 1024)) > 0){
// printf("Line: %s\n", buffer);
buffer[n] = '\0';
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?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 send() reads 1024 bytes from a 4-byte "ACK" string literal, causing buffer over-read

send(fd, "ACK", 1024, 0) specifies a length of 1024 bytes, but the string literal "ACK" is only 4 bytes. This is the same over-read pattern as the "get" send, causing undefined behavior and potential memory disclosure.

Suggested change
send(fd, "ACK", 1024, 0); //why are we sending this?
send(fd, "ACK", 3, 0);
Staging: Open in Devin

Was this helpful? React with 👍 or 👎 to provide feedback.

Debug

Playground

}
}
//editorOpen("test");
}
Comment on lines +1305 to +1327

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical: Protocol issues in receiveFile() will cause data corruption or hangs.

Multiple issues:

  1. Line 1310-1311: If read() returns -1 (error), buffer[-1] = '\0' causes undefined behavior.

  2. Line 1310: The first read() consumes "Start Transfer" but discards it without verification—correct intent but poor error handling.

  3. Lines 1314-1317: strcmp(buffer, "End Transfer") assumes TCP delivers exact message boundaries. TCP is a stream protocol; multiple sends may coalesce into one read() (e.g., "bye!\nEnd Transfer") or one send may split across multiple reads. This will fail to detect the sentinel.

  4. Line 1323: The comment asks "why are we sending this?" — it's for flow control, but it sends 1024 bytes for a 3-byte "ACK".

🐛 Proposed approach

For a robust solution, implement length-prefixed or delimiter-based framing. A minimal fix for the immediate bugs:

 void receiveFile(int fd){
 	ssize_t n;
 	FILE *file = fopen("transfer", "w");
+	if (!file) {
+		perror("fopen");
+		return;
+	}
 	char buffer[1024];

 	n = read(fd, buffer, 1024);
+	if (n <= 0) {
+		fclose(file);
+		return;
+	}
 	buffer[n] = '\0';
+	// Verify "Start Transfer" sentinel here if needed

 	while (1){
 		if ((n = read(fd, buffer, 1024)) > 0){
-			// printf("Line: %s\n", buffer);
 			buffer[n] = '\0';
 			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?
+			send(fd, "ACK", 3, 0);
+		} else {
+			break;  // Handle read error or EOF
 		}
 	}
-	//editorOpen("test");
+	fclose(file);
 }

Note: This still doesn't fix the TCP framing issue. For production use, implement proper message framing (e.g., newline-delimited with escaping, or length-prefix encoding).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@kilo.c` around lines 1305 - 1327, In receiveFile(): fix unsafe and
protocol-brittle reads by checking fopen() success and handling read() return
values (treat n==0 as peer closed, n<0 as error) before indexing buffer; remove
the stray initial unchecked read that discards data (or at least validate its
return) and instead use a single loop that accumulates bytes and searches across
chunk boundaries for the sentinel "End Transfer" (do not assume one read == one
message), write the raw bytes returned by read() to the FILE using fwrite/
fwrite-like semantics (do not append an extra '\n' unconditionally), and if you
need an acknowledgement keep the ACK length correct (send(fd, "ACK", 3, 0)) or
remove ACKs entirely if not part of the protocol.


/* ============================= 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;
}
Comment on lines +1339 to +1348

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Memory leak: res from getaddrinfo is never freed.

getaddrinfo() allocates a linked list that must be freed with freeaddrinfo() when no longer needed.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@kilo.c` around lines 1339 - 1348, The getaddrinfo call allocates a linked
list stored in res but the code never calls freeaddrinfo, leaking memory; after
you're done using res (e.g., after connecting or when exiting the setup block)
call freeaddrinfo(res) to release the allocation — ensure you free res in all
early-return/error paths (for example before returning 1 if you later check
connections) and consider using traverser only for iteration while keeping res
for the freeaddrinfo(res) call.


// 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 No check for connection failure: serverFd used uninitialized if all addresses fail

If all addresses returned by getaddrinfo fail to connect, the for loop exits with traverser == NULL. There is no check for this — the code proceeds to printf("Connected\n") and uses serverFd, which may hold a file descriptor from a failed socket() call or be completely uninitialized (if socket() also failed for the last address). This leads to undefined behavior when send()/read() are called on an invalid fd.

Suggested change
}
printf("Connected\n");
}
if (!traverser) {
fprintf(stderr, "Error: Could not connect to server.\n");
freeaddrinfo(res);
return 1;
}
printf("Connected\n");
Staging: Open in Devin

Was this helpful? React with 👍 or 👎 to provide feedback.

Debug

Playground

Comment on lines +1352 to +1359

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Missing connection failure check.

The loop iterates through addresses but doesn't verify that a connection was actually established. If all addresses fail, traverser becomes NULL and the code proceeds with an invalid serverFd, leading to undefined behavior.

🐛 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 == NULL) {
+		fprintf(stderr, "Error: Could not connect to server.\n");
+		return 1;
+	}
 	printf("Connected\n");
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@kilo.c` around lines 1352 - 1359, The connection loop over res/traverser may
leave serverFd invalid or leak sockets if none succeed; modify the loop so that
when socket() succeeds but connect() fails you close(serverFd) before
continuing, and after the loop check if traverser is NULL (or if no successful
connect occurred) and handle the failure (log error and return/exit with
non-zero) instead of proceeding to use serverFd; reference the traverser, res,
and serverFd variables and ensure only a successfully connected serverFd is used
after the loop.


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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 send() reads 1024 bytes from a 4-byte string literal, causing buffer over-read

send(serverFd, "get", 1024, 0) specifies a length of 1024 bytes, but the string literal "get" is only 4 bytes (including null terminator). This causes send() to read 1020 bytes past the end of the string literal, which is undefined behavior and can leak adjacent memory contents to the server.

Suggested change
send(serverFd, "get", 1024, 0);
send(serverFd, "get", 3, 0);
Staging: Open in Devin

Was this helpful? React with 👍 or 👎 to provide feedback.

Debug

Playground

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical: Buffer overread when sending "get" command.

send(serverFd, "get", 1024, 0) sends 1024 bytes but the string literal is only 4 bytes (including null). This reads uninitialized stack memory and sends it to the server.

🐛 Proposed fix
-			send(serverFd, "get", 1024, 0);
+			send(serverFd, "get", strlen("get"), 0);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
send(serverFd, "get", 1024, 0);
send(serverFd, "get", strlen("get"), 0);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@kilo.c` at line 1366, The send call is using an incorrect length causing a
buffer overread: replace send(serverFd, "get", 1024, 0) with a correct length
(e.g., send(serverFd, "get", sizeof("get")) or send(serverFd, "get",
strlen("get")+1, 0) or send(serverFd, "get", 3, 0) depending on whether you need
the NUL) to only transmit the actual bytes of the "get" command; update the call
where send(serverFd, "get", 1024, 0) appears so it no longer reads uninitialized
memory.

receiveFile(serverFd);
}
// printf("%s\n", buffer);
}

close(serverFd);
// exit(0);

//start editor
initEditor();
editorSelectSyntaxHighlight("transfer");
editorOpen(argv[1]);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 editorOpen(argv[1]) opens hostname instead of the downloaded "transfer" file

After downloading a file from the server and saving it as "transfer", the code calls editorOpen(argv[1]) which tries to open argv[1] — the server hostname — as a file. It should be editorOpen("transfer") to open the file that was just downloaded. The editorSelectSyntaxHighlight("transfer") on the line above correctly uses "transfer", confirming the intent.

Suggested change
editorOpen(argv[1]);
editorOpen("transfer");
Staging: Open in Devin

Was this helpful? React with 👍 or 👎 to provide feedback.

Debug

Playground

Comment on lines +1377 to 1378

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical: Opens wrong file in editor.

Line 1378 calls editorOpen(argv[1]) which is the host argument (e.g., "localhost"), not the downloaded file. The received content is written to "transfer" but the editor opens a non-existent file named after the hostname.

🐛 Proposed fix
     //start editor
 	initEditor();
     editorSelectSyntaxHighlight("transfer");
-    editorOpen(argv[1]);
+    editorOpen("transfer");
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
editorSelectSyntaxHighlight("transfer");
editorOpen(argv[1]);
editorSelectSyntaxHighlight("transfer");
editorOpen("transfer");
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@kilo.c` around lines 1377 - 1378, The editor is opening the wrong file:
editorOpen(argv[1]) uses the host argument instead of the downloaded file;
change the call to open the file where received content is saved (the "transfer"
buffer/file), i.e., call editorOpen with the same filename used when writing the
received data (the string "transfer" or the variable that holds that path), and
keep the prior editorSelectSyntaxHighlight("transfer") call so syntax and buffer
name match (update any variable name if the transfer filename is stored in a
variable).

enableRawMode(STDIN_FILENO);
editorSetStatusMessage(
Comment on lines +1362 to 1380

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 stdin is consumed by fgets loop, making the editor non-functional afterward

The while (fgets(buffer, 1024, stdin)) loop at line 1362 reads from stdin until EOF. After EOF, the code proceeds to start the editor (line 1376+), which also reads from STDIN_FILENO. Since stdin is already at EOF, editorReadKey() will call read(fd,&c,1) which returns 0 indefinitely (or -1), causing the editor to either spin or exit immediately. The editor phase is essentially dead code.

(Refers to lines 1362-1385)

Prompt for agents
The main() function first reads commands from stdin via fgets() until EOF, then tries to start the kilo editor which also reads from stdin. Once stdin reaches EOF, the editor cannot receive any keypresses and is non-functional. The design needs restructuring: either the command loop and the editor should not both use stdin, or the editor should be started before consuming stdin with fgets. One approach would be to use a separate mechanism for server commands (e.g., command-line arguments or a dedicated control channel) and reserve stdin for the editor's interactive use.
Staging: Open in Devin

Was this helpful? React with 👍 or 👎 to provide feedback.

Debug

Playground

Comment on lines +1362 to 1380

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 Editor code is unreachable after stdin EOF-driven command loop

The main function at kilo.c:1362-1370 reads commands from stdin in a while(fgets(...)) loop. This loop only exits when stdin reaches EOF (or an error). After the loop exits, the code at lines 1376-1385 attempts to start the editor, which calls enableRawMode(STDIN_FILENO) and then enters an infinite loop reading keystrokes from STDIN_FILENO via editorReadKey. If stdin was a pipe, it's now closed/at EOF, so read() in editorReadKey (kilo.c:260) will spin forever returning 0. If stdin was a terminal, the user would have had to send Ctrl-D, and the terminal may still work but all typed input during the command loop is lost. This design makes the editor portion effectively unreachable or broken in practice.

(Refers to lines 1362-1385)

Staging: Open in Devin

Was this helpful? React with 👍 or 👎 to provide feedback.

Debug

Playground

Expand Down
143 changes: 143 additions & 0 deletions server.cpp
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;
}
Comment on lines +23 to +31

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Hardcoded filename with no error checking.

readFile() opens a hardcoded file "test" without checking if the open succeeded. If the file doesn't exist, getline() will fail silently and return an empty vector.

🛡️ Proposed fix
-vector<string> readFile(){
+vector<string> readFile(const string& filename = "test"){
 	string line;
 	vector<string> lines;
-	ifstream file("test");
+	ifstream file(filename);
+	if (!file.is_open()) {
+		cerr << "Error: Cannot open file " << filename << endl;
+		return lines;
+	}
 	while (getline(file, line)){
 		lines.push_back(line);
 	}
 	return lines;
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@server.cpp` around lines 23 - 31, The readFile() function currently opens a
hardcoded ifstream file("test") and never checks whether the file opened
successfully; change readFile to accept a filename parameter (e.g.,
readFile(const string& filename)), open the file with ifstream file(filename),
verify the stream (file.is_open() or if (!file) ...) before calling getline, and
handle failures by returning an empty vector or propagating an error (throw or
error/log) so callers know the open failed; update any callers of readFile()
accordingly and ensure the getline loop only runs after the successful open.


//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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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

send(fd, (void*)"Start Transfer", 1024, 0) at line 37 sends 1024 bytes starting from a 15-byte string literal. send(fd, (void*)"End Transfer", 1024, 0) at line 55 sends 1024 bytes from a 13-byte string. Both read far past the end of the string literal, causing undefined behavior and potential information leakage.

Suggested change
send(fd, (void*)"Start Transfer", 1024, 0);
send(fd, (void*)"Start Transfer", strlen("Start Transfer"), 0);
Staging: Open in Devin

Was this helpful? React with 👍 or 👎 to provide feedback.

Debug

Playground

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical: Buffer overread when sending sentinel strings.

send(fd, (void*)"Start Transfer", 1024, 0) sends 1024 bytes, but the string literal is only 15 bytes (including null terminator). This reads uninitialized/arbitrary memory beyond the string, potentially leaking sensitive data to the client.

🐛 Proposed fix
-	send(fd, (void*)"Start Transfer", 1024, 0);
+	send(fd, "Start Transfer", strlen("Start Transfer"), 0);

Apply the same fix to line 55:

-	send(fd, (void*)"End Transfer", 1024, 0);
+	send(fd, "End Transfer", strlen("End Transfer"), 0);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
send(fd, (void*)"Start Transfer", 1024, 0);
send(fd, "Start Transfer", strlen("Start Transfer"), 0);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@server.cpp` at line 37, The send call is overshooting the actual string
length and causes a buffer overread; replace the fixed 1024 length in send(fd,
(void*)"Start Transfer", 1024, 0) with the correct length (e.g., use
strlen("Start Transfer")+1 or sizeof("Start Transfer") to include the NUL) and
apply the same change to the other similar call at line 55 so both sentinel
string sends use the actual string length rather than 1024.


/* 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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Buffer over-read: send reads 1024 bytes from "End Transfer" string literal

send(fd, (void*)"End Transfer", 1024, 0) specifies 1024 bytes, but the string literal is only 13 bytes. Same issue as above.

Suggested change
send(fd, (void*)"End Transfer", 1024, 0);
send(fd, (void*)"End Transfer", 12, 0);
Staging: Open in Devin

Was this helpful? React with 👍 or 👎 to provide feedback.

Debug

Playground

}
Comment on lines +34 to +56

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 Protocol design has no framing - relies on one-message-per-read assumption

The entire client-server protocol assumes that each read() returns exactly one complete message (e.g., exactly "Start Transfer", exactly one line, exactly "End Transfer"). TCP is a stream protocol with no message boundaries — read() can return partial messages or multiple messages concatenated together. The lock-step ACK mechanism (server.cpp:52 waits for ACK, kilo.c:1323 sends ACK) attempts to work around this but is fragile. Under real network conditions, messages can still be fragmented or coalesced, causing protocol desynchronization. This is a fundamental design issue that would need a proper framing protocol (length-prefix, delimiter-based, etc.).

Staging: Open in Devin

Was this helpful? React with 👍 or 👎 to provide feedback.

Debug

Playground


//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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Non-null-terminated buffer used to construct std::string in server thread

read(clientFd, buffer, 1024) at server.cpp:66 fills the buffer but does not null-terminate it. Then string line(buffer) at line 67 constructs a string by scanning for a null terminator, potentially reading garbage past the received data. The subsequent comparisons (line == "exit", line == "get") will fail or match incorrectly.

Suggested change
while ((n = read(clientFd, buffer, 1024)) > 0){
string line(buffer);
while ((n = read(clientFd, buffer, 1023)) > 0){
buffer[n] = '\0';
string line(buffer);
Staging: Open in Devin

Was this helpful? React with 👍 or 👎 to provide feedback.

Debug

Playground


Comment on lines +66 to +68

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Potential buffer not null-terminated.

If read() returns exactly 1024 bytes, buffer won't be null-terminated. Constructing std::string line(buffer) then reads past the buffer looking for the null terminator, causing undefined behavior.

🐛 Proposed fix
 	while ((n = read(clientFd, buffer, 1024)) > 0){
-		string line(buffer);
+		string line(buffer, n);  // Use constructor that takes length
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@server.cpp` around lines 66 - 68, The code constructs std::string
line(buffer) after ssize_t n = read(clientFd, buffer, 1024), which is unsafe
when read returns 1024 because buffer isn't null-terminated; change the
construction to use the explicit length returned by read (e.g., construct line
from buffer with length n) or ensure buffer[n] = '\0' after verifying n >= 0 and
n < buffer_size; update uses around variables read, clientFd, buffer, n and
replace string line(buffer) with a length-aware construction to avoid reading
past the buffer.

if (line == "exit"){
close(clientFd);
}
else if (line == "get"){
cout << "Get Received" << endl;
sendFile(clientFd, readFile());
}
}
return NULL;
}

//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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Uninitialized val used in setsockopt for SO_REUSEADDR

int val; is declared at line 99 but never initialized. It is then passed to setsockopt(serverFd, SOL_SOCKET, SO_REUSEADDR, &val, sizeof(int)). SO_REUSEADDR requires a non-zero value to be enabled. Using an uninitialized variable is undefined behavior and the option may not actually be enabled if val happens to be 0.

Suggested change
int val;
if (setsockopt(serverFd, SOL_SOCKET, SO_REUSEADDR, &val, sizeof(int))){
int val = 1;
if (setsockopt(serverFd, SOL_SOCKET, SO_REUSEADDR, &val, sizeof(int))){
Staging: Open in Devin

Was this helpful? React with 👍 or 👎 to provide feedback.

Debug

Playground

std::cerr << "Error: Can't reuse socket." << std::endl;
return 2;
}
Comment on lines +99 to +103

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical: Uninitialized variable passed to setsockopt.

The variable val is declared but never initialized before being passed to setsockopt(). This results in undefined behavior. SO_REUSEADDR expects a non-zero value to enable the option.

🐛 Proposed fix
 	// Set options for socket
-	int val;
+	int val = 1;
 	if (setsockopt(serverFd, SOL_SOCKET, SO_REUSEADDR, &val, sizeof(int))){
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
int val;
if (setsockopt(serverFd, SOL_SOCKET, SO_REUSEADDR, &val, sizeof(int))){
std::cerr << "Error: Can't reuse socket." << std::endl;
return 2;
}
int val = 1;
if (setsockopt(serverFd, SOL_SOCKET, SO_REUSEADDR, &val, sizeof(int))){
std::cerr << "Error: Can't reuse socket." << std::endl;
return 2;
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@server.cpp` around lines 99 - 103, The code passes an uninitialized int (val)
into setsockopt for SO_REUSEADDR; initialize it to a non-zero value (e.g., 1)
before the call and pass its address and correct sizeof (use the same variable
name for sizeof) so setsockopt receives a defined true value; update the symbols
val and the setsockopt(serverFd, SOL_SOCKET, SO_REUSEADDR, &val, sizeof(int))
invocation accordingly to use the initialized variable.


// 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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 delete instead of delete[] for array allocated with new[]

name is allocated with new char[1024] at line 122 but freed with delete name instead of delete[] name at line 130. Using delete on an array allocated with new[] is undefined behavior per the C++ standard.

Suggested change
delete name;
delete[] name;
Staging: Open in Devin

Was this helpful? React with 👍 or 👎 to provide feedback.

Debug

Playground

Comment on lines +122 to +130

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Mismatching allocation and deallocation.

name is allocated with new char[1024] (array form) but deallocated with delete name (scalar form). This is undefined behavior.

🐛 Proposed fix
 	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;
+	delete[] name;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
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;
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;
🧰 Tools
🪛 Cppcheck (2.20.0)

[error] 130-130: Mismatching allocation and deallocation

(mismatchAllocDealloc)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@server.cpp` around lines 122 - 130, The buffer "name" is allocated with array
new (new char[1024]) but freed with scalar delete (delete name), causing
undefined behavior; change the deallocation to delete[] name or, better, replace
the manual buffer with a std::string or std::vector<char> and use that with
gethostname to avoid manual new/delete—update the code around the gethostname
call and the delete to use delete[] name or the chosen RAII container (e.g.,
std::string nameBuf or std::vector<char>), and ensure the cout still prints the
correct C-string via nameBuf.c_str() or equivalent.


// Connect a client
struct sockaddr_in cliAddr;
len = sizeof(cliAddr);
while (true){
int clientFd = accept(serverFd, (struct sockaddr *)&serverAddr, &len);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 accept() overwrites serverAddr instead of writing to cliAddr

At server.cpp:136, accept(serverFd, (struct sockaddr *)&serverAddr, &len) uses &serverAddr as the output address, but cliAddr was declared at line 133 for this purpose. This overwrites the server's own address structure with the client's address, corrupting server state. Should be (struct sockaddr *)&cliAddr.

Suggested change
int clientFd = accept(serverFd, (struct sockaddr *)&serverAddr, &len);
int clientFd = accept(serverFd, (struct sockaddr *)&cliAddr, &len);
Staging: Open in Devin

Was this helpful? React with 👍 or 👎 to provide feedback.

Debug

Playground


// Create thread to deal with client
pthread_t thread;
pthread_create(&thread, NULL, threadFunc, (void *)&clientFd);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Race condition: pointer to loop-local clientFd passed to thread

clientFd is a local variable inside the while(true) loop. Its address is passed to pthread_create at line 140. By the time the new thread dereferences the pointer at server.cpp:61 (int clientFd = *(int*)args), the main thread may have already looped back and overwritten clientFd with a new accept() result. This is a classic TOCTOU race that can cause threads to operate on the wrong file descriptor.

Prompt for agents
In server.cpp main(), clientFd is a stack variable in the while loop and its address is passed to pthread_create. The spawned thread reads *(int*)args to get the fd, but by then the main thread may have overwritten clientFd with a new accept() result. Fix by either: (1) heap-allocating an int for each client (int *fdp = new int; *fdp = clientFd; pthread_create(..., fdp); and free in thread), or (2) casting the int to intptr_t and passing it as the void* directly.
Staging: Open in Devin

Was this helpful? React with 👍 or 👎 to provide feedback.

Debug

Playground

}
Comment on lines +135 to +141

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical: Race condition passing &clientFd to thread and wrong sockaddr in accept.

Two issues here:

  1. Wrong address struct: accept() uses &serverAddr instead of &cliAddr. This overwrites the server's address structure.

  2. Race condition: &clientFd is a pointer to a stack variable in the loop. If a new client connects before the thread reads the value, clientFd gets overwritten, causing both threads to serve the same (or wrong) client.

🐛 Proposed fix
 	while (true){
-		int clientFd = accept(serverFd, (struct sockaddr *)&serverAddr, &len);
+		int clientFd = accept(serverFd, (struct sockaddr *)&cliAddr, &len);

 		// Create thread to deal with client
 		pthread_t thread;
-		pthread_create(&thread, NULL, threadFunc, (void *)&clientFd);
+		int *clientFdPtr = new int(clientFd);
+		pthread_create(&thread, NULL, threadFunc, (void *)clientFdPtr);
 	}

Then in threadFunc, free the allocated memory:

 void *threadFunc(void *args){
 	ssize_t n;
 	int clientFd = *(int*)args;
+	delete (int*)args;
 	char buffer[1024];
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@server.cpp` around lines 135 - 141, accept() is incorrectly writing into
serverAddr and you’re passing a pointer to the loop-local clientFd into
pthread_create which causes a race; change the accept call to use a client
sockaddr (cliAddr) and a separate socklen_t (e.g., cliLen) instead of
&serverAddr, and instead of passing &clientFd allocate an int on the heap for
each accepted socket (e.g., int *pclient = malloc(sizeof(int)); *pclient =
clientFd;) and pass pclient to pthread_create; finally, modify threadFunc to
accept and use the heap pointer and free(pclient) inside threadFunc when done to
avoid leaks and races.

return 0;
}
4 changes: 4 additions & 0 deletions test
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!
Empty file added transfer
Empty file.