Browse files

Don't assume that data will come all in one read().

It was easier when we could assume this. Unfortunately, either ssh
clients don't send data all in one write(), or cygwin messes around
with the sockets. Either way, messages can come in multiple read()'s
and we now cope with that.
  • Loading branch information...
1 parent 82ec14e commit 3a83d68eea563111cd459ee849032d049bbead10 @wesleyd committed Nov 25, 2009
Showing with 276 additions and 70 deletions.
  1. +4 −11 TODO
  2. +227 −29 charade.c
  3. +3 −0 eprintf.h
  4. +28 −25 pageant.c
  5. +14 −5 pageant.h
View
15 TODO
@@ -1,15 +1,8 @@
-o Messages can come off the socket in two parts. Darn. Work properly
- when we don't get a full message.
o Copyright stuff
-o Install as ssh-agent
-o Work with keychain
-o Rationalise debug output
-o TODO() format + exit macro
-o Deal with partial messages from ssh - unlikely, but catastrophic!
- + Look at length at beginning, block until length arrives.
+o Work on debug output
o Check open files in each process that results
o Signal handling
o Specified agent socket
-o Redirect stderr (& stdout?) to log even 'pon fork
-o Deal with revents=0x8
-o Why don't we remove ssh directory when finished???
+o Redirect stderr (& stdout?) to *not* log if told not to
+o Do we remove ssh directory when finished??? (Signals!)
+o Check 64-bit cygwin
View
256 charade.c
@@ -28,6 +28,9 @@
#define LISTEN_BACKLOG 5
#define BUFSIZE 8192
+// I picked this number out of my ass. It's, like, a sanity check.
+#define CRAZY_MAX_MESSAGE_SIZE 65535
+
// TODO: is there actually something magical about this number?
#define AGENT_COPYDATA_ID 0x804e50ba
@@ -41,6 +44,7 @@ struct socklist_node_t {
TAILQ_ENTRY(socklist_node_t) next;
int fd;
byte *data;
+ size_t len;
};
TAILQ_HEAD(socklist_queue, socklist_node_t) socklist;
@@ -62,6 +66,7 @@ add_socket_to_socket_list(int sock)
p->fd = sock;
p->data = NULL;
+ p->len = 0;
EPRINTF(5, "adding socket %d to socket list.\n", sock);
@@ -308,12 +313,15 @@ void
fd_is_closed(int fd)
{
EPRINTF(3, "Removing fd %d from list.\n", fd);
- // Remove it from the list...
- // Remove the fd from the list...
struct socklist_node_t *p;
for (p = socklist.tqh_first; p != NULL; p = p->next.tqe_next) {
if (p->fd == fd) {
+
+ free(p->data);
+ p->data = NULL;
+ p->len = 0;
+
TAILQ_REMOVE(&socklist, p, next);
break;
}
@@ -323,54 +331,244 @@ fd_is_closed(int fd)
close(fd);
}
-void
-deal_with_ready_fds(struct pollfd *fds, int nfds)
-{
- EPRINTF(3, "nfds=%d.\n", nfds);
-
- int i;
- for (i = 0; i < nfds; ++i) {
- if (!fds[i].revents)
- continue;
-
- // Don't just *assume* that entry 0 is listen_sock...
- if (listen_sock == fds[i].fd) {
- accept_new_socket();
- } else if (fds[i].revents & POLLIN) {
- // Deal with data from fds[i].fd...
- const size_t count = BUFSIZE;
- byte buf[BUFSIZE];
+/*
ssize_t numbytes = read(fds[i].fd, buf, count);
if (0 == numbytes) {
close(fds[i].fd);
fd_is_closed(fds[i].fd);
+ continue;
} else if (-1 == numbytes) {
// TODO: Should we handle EAGAIN/EWOULDBLOCK specially?
EPRINTF(0, "internal error: read(fd=%d) => errno=%d/%s.\n",
fds[i].fd, errno, strerror(errno));
close(fds[i].fd);
fd_is_closed(fds[i].fd);
+ continue;
+*/
+
+struct socklist_node_t *
+socklist_node_from_fd(int fd)
+{
+ struct socklist_node_t *p;
+ for (p = socklist.tqh_first; p != NULL; p = p->next.tqe_next) {
+ if (p->fd == fd) {
+ return p;
+ }
+ }
+ return NULL;
+}
+
+// Return true if the socket - based on the data we've read from
+// it so far - can never, ever contain a valid message.
+int
+socket_will_never_contain_message(struct socklist_node_t *np)
+{
+ if (np->len > CRAZY_MAX_MESSAGE_SIZE) { // OK, waaaay too much data
+ return 1;
+ }
+
+ if (np->len < 4) { // Too soon to tell
+ return 0;
+ }
+
+ unsigned int claimed_numbytes = GET_32BIT(np->data);
+ if (claimed_numbytes > CRAZY_MAX_MESSAGE_SIZE) {
+ return 1;
+ }
+
+ return 0;
+}
+
+
+// Read data from np->fd into np->buf, expanding np->buf as necessary.
+// Return zero for success.
+// Non-zero means some sort of failure, and caller should close socket etc.
+int
+read_data_for_node(struct socklist_node_t *np)
+{
+ if (!np) {
+ EPRINTF(0, "null pointer!");
+ return -1;
+ }
+
+ int data_still_to_be_read = 1;
+
+ // byte buf[BUFSIZE];
+ byte buf[3];
+
+ while (data_still_to_be_read) {
+ // Protect against infinite memory consumption...
+ if(socket_will_never_contain_message(np)) {
+ EPRINTF(0, "socket %d will never contain message.\n", np->fd);
+ return -1;
+ }
+
+ ssize_t numbytes = read(np->fd, buf, sizeof buf);
+
+ if (0 == numbytes) {
+ EPRINTF(1, "read zero bytes from socket. Must have closed.\n");
+ return -1;
+ } else if (-1 == numbytes) {
+ if (EAGAIN == errno || EWOULDBLOCK == errno) {
+ EPRINTF(5, "read(fd=%d) => EAGAIN/EWOULDBLOCK: errno=%d.\n",
+ np->fd, errno);
+ // No problem, this just means we've exhausted the socket...
+ return 0;
+ } else if (EINTR == errno) {
+ EPRINTF(0, "read(fd=%d) => EINTR: retrying.\n", np->fd);
+ continue;
} else {
- int retlen = send_request_to_pageant(buf, numbytes, sizeof buf);
+ EPRINTF(0, "internal error: read(fd=%d) => -1/errno=%d: %s.\n",
+ np->fd, errno, strerror(errno));
+ return -1;
+ }
+ } else if (numbytes < 0) {
+ EPRINTF(0, "weird!!! read(fd=%d) returned %d. Giving up.\n",
+ np->fd, (int) numbytes);
+ return -1;
+ } else {
+ // Normal case: there was data to be read, and we read it...
+ // numbytes is positive (and not zero either!)
+
+ if (numbytes < sizeof(buf)) {
+ data_still_to_be_read = 0;
+ } else if (numbytes > sizeof(buf)) {
+ EPRINTF(0, "Aiee! Got more bytes than buffer! (%d>%d)\n",
+ numbytes, sizeof(buf));
+ return -1;
+ } else { // numbytes == sizeof(buf)
+ data_still_to_be_read = 1;
+ }
+
+ // Right, now put the data in a sensible place...
+ np->data = realloc(np->data, np->len + numbytes);
+ if (!np->data) {
+ EPRINTF(0, "realloc failure (%lu -> %lu)!\n",
+ (long unsigned) np->len,
+ (long unsigned) (np->len + numbytes));
+ return -1;
+ }
+ memcpy(np->data + np->len, buf, numbytes);
+ np->len += numbytes;
+ }
+ }
+
+ return 0;
+}
+
+int
+socket_contains_full_message(struct socklist_node_t *np)
+{
+ if (np->len < 4) {
+ return 0;
+ }
+
+ // If np->len is non-zero, then np->data is non-null...
+ unsigned int claimed_numbytes = GET_32BIT(np->data);
+
+ // The message length in the message doesn't include the four
+ // bytes of the length itself.
+ if (claimed_numbytes + 4 < np->len) {
+ return 0;
+ }
+
+ return 1;
+}
+
+void
+deal_with_ready_fds(struct pollfd *fds, int nfds)
+{
+ EPRINTF(3, "we have %d fds in total\n", nfds);
+
+ int i;
+ for (i = 0; i < nfds; ++i) {
+ const int fd = fds[i].fd;
+ const short revents = fds[i].revents;
+
+ EPRINTF(5, "What about fd %d (revents = 0x%hx)?\n", fd, revents);
+
+ if (!revents)
+ continue;
+
+ // Don't just *assume* that entry 0 is listen_sock...
+ if (listen_sock == fd) {
+ accept_new_socket();
+ continue;
+ } else if (revents & POLLHUP) {
+ close(fd); // Probably unnecessary, but harmless. (?)
+ fd_is_closed(fd);
+ continue;
+ } else if (revents & POLLIN) {
+ struct socklist_node_t *np = socklist_node_from_fd(fd);
+
+ if (!np) {
+ EPRINTF(0, "no such get_socklist_node for fd %d!\n", fd);
+ continue;
+ }
+
+ if (read_data_for_node(np)) {
+ // This includes if the socket has just closed...
+ EPRINTF(2, "Error reading data for fd %d. Closing.\n", fd);
+ close(fd);
+ fd_is_closed(fd);
+ continue;
+ }
+
+ if (socket_will_never_contain_message(np)) {
+ EPRINTF(0, "Giving up on fd %d.\n", fd);
+ close(fd);
+ fd_is_closed(fd);
+ continue;
+ }
+
+ if (socket_contains_full_message(np)) {
+ EPRINTF(5, "Socket %d contains (at least) a full message.\n",
+ np->fd);
+
+ byte outbuf[BUFSIZE];
+
+ // Note, we *assume* that there won't be *more* than one
+ // message in np...
+
+ int retlen = send_request_to_pageant(np->data, np->len,
+ outbuf, sizeof outbuf);
+ if (retlen <= 0) {
+ // Error sending message to pageant...
+ EPRINTF(0, "Error sending %d-byte message to pageant.\n",
+ np->len);
+ close(fd);
+ fd_is_closed(fd);
+ continue;
+ }
+
+ free(np->data);
+ np->data = NULL;
+ np->len = 0;
// Now, send buf back to the socket. We should probably
// loop and retry or use poll properly since it's nonblocking...
- ssize_t byteswritten = write(fds[i].fd, buf, retlen);
+ ssize_t byteswritten = write(fd, outbuf, retlen);
if (byteswritten != retlen) {
- EPRINTF(0, "Tried to write %d bytes, "
- "ended up writing %d.\n",
- retlen, byteswritten);
- close(fds[i].fd);
- fd_is_closed(fds[i].fd);
+ EPRINTF(0, "Tried to write %d bytes back to ssh client, "
+ "ended up writing %d (errno is %d).\n",
+ retlen, byteswritten, errno);
+ close(fd);
+ fd_is_closed(fd);
}
+ } else {
+ EPRINTF(5, "Socket %d has %d bytes, but not a full message.\n",
+ np->fd, np->len);
}
+
+
+
} else {
EPRINTF(0, "Don't know how to deal with revents=0x%x on fd %d.\n",
- fds[i].revents, fds[i].fd);
- close(fds[i].fd);
- fd_is_closed(fds[i].fd);
+ revents, fd);
+ close(fd);
+ fd_is_closed(fd);
}
}
}
View
3 eprintf.h
@@ -12,6 +12,9 @@ extern "C" {
#define EPRINTF(Level, Format, Args...) \
eprintf(Level, "%s: " Format, __func__, ##Args)
+#define EPRINTF_RAW(Level, Format, Args...) \
+ eprintf(Level, Format, ##Args)
+
int eprintf(int level, const char *fmt, ...)
__attribute__ ((format (printf, 2, 3)));
View
53 pageant.c
@@ -17,41 +17,41 @@
#define AGENT_MAX_MSGLEN 8192
-#define GET_32BIT_MSB_FIRST(cp) \
- (((unsigned long)(unsigned char)(cp)[0] << 24) | \
- ((unsigned long)(unsigned char)(cp)[1] << 16) | \
- ((unsigned long)(unsigned char)(cp)[2] << 8) | \
- ((unsigned long)(unsigned char)(cp)[3] ))
+void
+print_buf(int level, byte *buf, int numbytes)
+{
+ int i;
+ for (i = 0; i < numbytes;) {
+ EPRINTF_RAW(level, "%02x ", buf[i]);
+ ++i;
+ if (!(i%8)) EPRINTF_RAW(level, " ");
+ if (!(i%16) || i == numbytes) EPRINTF_RAW(level, "\n");
+ }
+}
-#define GET_32BIT(cp) GET_32BIT_MSB_FIRST(cp)
+
+// "in" means "to pageant", "out" means "from pageant". Sorry.
int
-send_request_to_pageant(byte *buf, int numbytes, int bufsize)
+send_request_to_pageant(byte *inbuf, int inbytes, byte *outbuf, int outbuflen)
{
- EPRINTF(3, "Sending %d bytes to pageant.\n", numbytes);
+ EPRINTF(3, "Sending %d bytes to pageant.\n", inbytes);
- if (numbytes < 4) {
- EPRINTF(0, "Pageant-bound message too short (%d bytes).\n", numbytes);
+ if (inbytes < 4) {
+ EPRINTF(0, "Pageant-bound message too short (%d bytes).\n", inbytes);
return 0;
}
- int ostensible_numbytes = GET_32BIT(buf);
- if (numbytes != ostensible_numbytes + 4) {
+ int claimed_inbytes = GET_32BIT(inbuf);
+ if (inbytes != claimed_inbytes + 4) {
EPRINTF(0, "Pageant-bound message is %d bytes long, but it "
"*says* it has %d=%d+4 bytes in it.\n",
- numbytes, ostensible_numbytes+4, ostensible_numbytes);
+ inbytes, claimed_inbytes+4, claimed_inbytes);
return 0;
}
- int i;
- for (i = 0; i < numbytes;) {
- fprintf(stderr, "%02x ", buf[i]);
- ++i;
- if (!(i%8)) fprintf(stderr, " ");
- if (!(i%16) || i == numbytes) fprintf(stderr, "\n");
- }
+ EPRINTF(5, "Message to pageant (%d bytes):\n", inbytes);
+ print_buf(5, inbuf, inbytes);
- // Now, let's just *assume* that it'll arrive in one big
- // chunk and just *send* it to pageant...
HWND hwnd;
hwnd = FindWindow("Pageant", "Pageant");
if (!hwnd) {
@@ -72,7 +72,7 @@ send_request_to_pageant(byte *buf, int numbytes, int bufsize)
}
byte *shmem = MapViewOfFile(filemap, FILE_MAP_WRITE, 0, 0, 0);
- memcpy(shmem, buf, numbytes);
+ memcpy(shmem, inbuf, inbytes);
COPYDATASTRUCT cds;
cds.dwData = AGENT_COPYDATA_ID;
cds.cbData = 1 + strlen(mapname);
@@ -82,12 +82,15 @@ send_request_to_pageant(byte *buf, int numbytes, int bufsize)
int retlen = 0;
if (id > 0) {
retlen = 4 + GET_32BIT(shmem);
- if (retlen > bufsize) {
+ if (retlen > outbuflen) {
EPRINTF(0, "Buffer too small to contain reply from pageant.\n");
return 0;
}
- memcpy(buf, shmem, retlen);
+ memcpy(outbuf, shmem, retlen);
+
+ EPRINTF(5, "Reply from pageant (%d bytes):\n", retlen);
+ print_buf(5, outbuf, retlen);
} else {
EPRINTF(0, "Couldn't SendMessage().\n");
return 0;
View
19 pageant.h
@@ -9,10 +9,19 @@
* Copyright (c) 2009, Wesley Darlington. All Rights Reserved.
*/
-// Send a numbytes-sized message (in buf) to pageant, synchronously.
-// Up to numbytes will be written into buf of the returned message. If
-// the returned message would overflow buf, or if any other error occurs,
-// 0 will be returned.
-int send_request_to_pageant(byte *buf, int numbytes, int bufsize);
+#define GET_32BIT_MSB_FIRST(cp) \
+ (((unsigned long)(unsigned char)(cp)[0] << 24) | \
+ ((unsigned long)(unsigned char)(cp)[1] << 16) | \
+ ((unsigned long)(unsigned char)(cp)[2] << 8) | \
+ ((unsigned long)(unsigned char)(cp)[3] ))
+
+#define GET_32BIT(cp) GET_32BIT_MSB_FIRST(cp)
+
+// Send a numbytes-sized message to pageant, synchronously.
+// Up to outbuflen bytes of response will be written into outbuf.
+// If the returned message would overflow buf, or if any other
+// error occurs, 0 will be returned.
+int send_request_to_pageant(byte *inbuf, int inbytes,
+ byte *outbuf, int outbuflen);
#endif // #ifndef PAGEANT_H

0 comments on commit 3a83d68

Please sign in to comment.