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

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.

🔴 receiveFile() uses read() return value as index without checking for error (-1)

At kilo.c:1310-1311, n = read(fd, buffer, 1024) may return -1 on error, but the code immediately does buffer[n] = '\0' without checking. When n == -1, this writes to buffer[-1], an out-of-bounds memory write.

Suggested change
n = read(fd, buffer, 1024);
buffer[n] = '\0';
n = read(fd, buffer, 1023);
if (n <= 0) { fclose(file); return; }
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 4-byte string literal "ACK", 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 reads 1020 bytes past the literal, leaking memory contents over the network.

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

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

Debug

Playground

}
}
//editorOpen("test");
}

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

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 after address traversal loop

After the for loop at kilo.c:1352-1358, there is no check whether traverser is NULL (meaning all connection attempts failed). The code unconditionally prints "Connected" and proceeds to use serverFd, which may hold a socket from a failed connect() call or be uninitialized if all socket() calls failed.

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


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 4-byte string literal "get", 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 of adjacent read-only memory, leaking process memory contents over the network (similar in nature to Heartbleed). The same pattern exists at kilo.c:1323 with "ACK".

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

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

Debug

Playground

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 transferred file "transfer"

editorOpen(argv[1]) opens the file named by the first argument, which is the server hostname (e.g., "localhost"). The transferred file is written to "transfer" (see kilo.c:1307), and syntax highlighting is set for "transfer" at kilo.c:1377, but the editor opens the wrong file. This should be editorOpen("transfer").

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

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

Debug

Playground

enableRawMode(STDIN_FILENO);
editorSetStatusMessage(
Comment on lines +1375 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 after fgets loop is unreachable

The while (fgets(buffer, 1024, stdin)) loop at kilo.c:1362 reads from stdin until EOF. After EOF, the code at lines 1376-1385 attempts to start the editor, which also reads from stdin via editorProcessKeypress(STDIN_FILENO). Since stdin has already reached EOF, the editor will not be able to read any keypresses. The entire editor section (initEditor, editorOpen, enableRawMode, the main editor loop) is effectively dead code in the current flow. The architecture seems fundamentally broken — the client reads commands interactively, then tries to start an interactive editor on the same already-exhausted stdin.

(Refers to lines 1375-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;
}

//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 15-byte "Start Transfer" literal

send(fd, (void*)"Start Transfer", 1024, 0) sends 1024 bytes but the string literal is only 15 bytes. This reads 1009 bytes of unrelated memory, leaking process memory to the client. Same issue at server.cpp:55 with "End Transfer" (13 bytes).

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

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

Debug

Playground


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

🔴 send() reads 1024 bytes from 13-byte "End Transfer" literal

send(fd, (void*)"End Transfer", 1024, 0) sends 1024 bytes but the string literal is only 13 bytes. This reads past the literal into unrelated memory.

Suggested change
send(fd, (void*)"End Transfer", 1024, 0);
send(fd, "End Transfer", strlen("End Transfer"), 0);
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.

🔴 Buffer from read() not null-terminated before constructing std::string

At server.cpp:66-67, read(clientFd, buffer, 1024) does not null-terminate buffer, but string line(buffer) constructs a string from a C-string expecting a null terminator. This reads past the valid data into uninitialized buffer memory, producing garbage in the string and potentially reading out of bounds.

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


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.

🔴 setsockopt(SO_REUSEADDR) uses uninitialized val variable

int val; at server.cpp:99 is never initialized. setsockopt(serverFd, SOL_SOCKET, SO_REUSEADDR, &val, sizeof(int)) at line 100 reads this uninitialized memory. SO_REUSEADDR requires a non-zero value to enable the option. If val happens to be 0, the option won't be set; otherwise it's undefined behavior.

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

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

// 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() writes client address into serverAddr, corrupting server's own address

accept(serverFd, (struct sockaddr *)&serverAddr, &len) passes &serverAddr instead of the declared &cliAddr (declared at server.cpp:133). This overwrites the server's address structure with the client's address. Additionally, len was set from sizeof(cliAddr) at line 134 which happens to be the same size, but semantically cliAddr should be used.

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

pthread_create(&thread, NULL, threadFunc, (void *)&clientFd) passes the address of clientFd, a variable local to the while(true) loop body at server.cpp:136. The new thread dereferences this pointer at server.cpp:61, but by that time the main thread may have already looped back and overwritten clientFd with the next accept() result. The thread may read the wrong file descriptor.

Prompt for agents
In server.cpp main() at line 140, a pointer to the local variable clientFd is passed to the new thread. Since the main thread immediately continues to the next accept() call, the clientFd value may be overwritten before the new thread reads it at line 61. Fix this by either: (1) heap-allocating the fd (int *fdPtr = new int(clientFd)) and freeing it in the thread, or (2) casting the int value directly to void* (using intptr_t) and casting back in the thread function.
Staging: Open in Devin

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

Debug

Playground

}
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.