simono41/shellinabox
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity

Fixes for some defects found by Coverity

Browse source 
* Added more checks for return values and null pointers.
* Removed some dead code and unused variables.
* Fixed handling of calls to exec() family functions. If this functions
  fail we need to cleanup resources.
This commit is contained in:
KLuka 2015-05-31 13:04:00 +02:00
parent 21c8d8e0b7
commit b3309b23d8
7 changed files with 69 additions and 55 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

10
libhttp/httpconnection.c
View file

@ -183,18 +183,13 @@ static ssize_t httpWrite(struct HttpConnection *http, const char *buf,
static int httpShutdown(struct HttpConnection *http, int how) { static int httpShutdown(struct HttpConnection *http, int how) {
if (http->sslHndl) { if (http->sslHndl) {
int rc = 0;
if (how != SHUT_RD) { if (how != SHUT_RD) {
dcheck(!ERR_peek_error()); dcheck(!ERR_peek_error());
for (int i = 0; i < 10; i++) { for (int i = 0; i < 10; i++) {
sslBlockSigPipe(); sslBlockSigPipe();
rc = SSL_shutdown(http->sslHndl); int rc = SSL_shutdown(http->sslHndl);
int sPipe = sslUnblockSigPipe(); int sPipe = sslUnblockSigPipe();
if (rc > 0) { if (rc < 1) {
rc = 0;
break;
} else {
rc = -1;
// Retry a few times in order to prefer a clean bidirectional // Retry a few times in order to prefer a clean bidirectional
// shutdown. But don't bother if the other side already closed // shutdown. But don't bother if the other side already closed
// the connection. // the connection.
@ -1776,6 +1771,7 @@ void httpSendReply(struct HttpConnection *http, int code,
response = stringPrintf(response, "%s", body); response = stringPrintf(response, "%s", body);
} }
free(body); free(body);
check(response);
httpTransfer(http, response, strlen(response)); httpTransfer(http, response, strlen(response));
if (code != 200 || isHead) { if (code != 200 || isHead) {
httpCloseRead(http); httpCloseRead(http);

2
libhttp/server.c
View file

@ -338,7 +338,7 @@ void destroyServer(struct Server *server) {
if (server) { if (server) {
if (server->serverFd >= 0) { if (server->serverFd >= 0) {
info("Shutting down server"); info("Shutting down server");
close(server->serverFd); NOINTR(close(server->serverFd));
} }
for (int i = 0; i < server->numConnections; i++) { for (int i = 0; i < server->numConnections; i++) {
server->connections[i].destroyConnection(server->connections[i].arg); server->connections[i].destroyConnection(server->connections[i].arg);

26
libhttp/ssl.c
View file

@ -368,33 +368,37 @@ int serverSupportsSSL(void) {
#if defined(HAVE_OPENSSL) #if defined(HAVE_OPENSSL)
static void sslGenerateCertificate(const char *certificate, static void sslGenerateCertificate(const char *certificate,
const char *serverName) { const char *serverName) {
debug("Auto-generating missing certificate \"%s\" for \"%s\"", debug("Auto-generating missing certificate \"%s\" for \"%s\"",
certificate, serverName); certificate, serverName);
pid_t pid = fork(); pid_t pid = fork();
if (pid == -1) { if (pid == -1) {
warn("Failed to generate self-signed certificate \"%s\"", certificate); warn("Failed to generate self-signed certificate \"%s\"", certificate);
} else if (pid == 0) { } else if (pid == 0) {
int fd = NOINTR(open("/dev/null", O_RDONLY)); int fd = NOINTR(open("/dev/null", O_RDONLY));
check(fd != -1); check(fd != -1);
check(NOINTR(dup2(fd, STDERR_FILENO)) == STDERR_FILENO); check(NOINTR(dup2(fd, STDERR_FILENO)) == STDERR_FILENO);
check(NOINTR(close(fd)) == 0); check(NOINTR(close(fd)) == 0);
fd = NOINTR(open("/dev/null", O_WRONLY)); fd = NOINTR(open("/dev/null", O_WRONLY));
check(fd != -1); check(fd != -1);
check(NOINTR(dup2(fd, STDIN_FILENO)) == STDIN_FILENO); check(NOINTR(dup2(fd, STDIN_FILENO)) == STDIN_FILENO);
check(NOINTR(close(fd)) == 0); check(NOINTR(close(fd)) == 0);
umask(077); umask(077);
check(setenv("PATH", "/usr/bin:/usr/sbin", 1) == 0); check(setenv("PATH", "/usr/bin:/usr/sbin", 1) == 0);
execlp("openssl", "openssl", "req", "-x509", "-nodes", "-days", "7300", char *subject;
"-newkey", "rsa:2048", "-keyout", certificate, "-out", certificate, check(subject = stringPrintf(NULL, "/CN=%s/", serverName));
"-subj", stringPrintf(NULL, "/CN=%s/", serverName), if (execlp("openssl", "openssl", "req", "-x509", "-nodes", "-days", "7300",
(char *)NULL); "-newkey", "rsa:2048", "-keyout", certificate, "-out", certificate,
check(0); "-subj", subject, (char *)NULL) < 0) {
warn("Failed to generate self-signed certificate \"%s\"", certificate);
free(subject);
}
} else { } else {
int status; int status;
check(NOINTR(waitpid(pid, &status, 0)) == pid); check(NOINTR(waitpid(pid, &status, 0)) == pid);
if (!WIFEXITED(status) || WEXITSTATUS(status) != 0) if (!WIFEXITED(status) || WEXITSTATUS(status) != 0) {
warn("Failed to generate self-signed certificate \"%s\"", certificate); warn("Failed to generate self-signed certificate \"%s\"", certificate);
}
} }
} }

1
libhttp/url.c
View file

@ -346,6 +346,7 @@ void initURL(struct URL *url, const struct HttpConnection *http,
url->url = NULL; url->url = NULL;
initHashMap(&url->args, urlDestroyHashMapEntry, NULL); initHashMap(&url->args, urlDestroyHashMapEntry, NULL);
if (!strcmp(http->method, "GET")) { if (!strcmp(http->method, "GET")) {
check(url->query);
urlParseQueryString(&url->args, url->query, strlen(url->query)); urlParseQueryString(&url->args, url->query, strlen(url->query));
} else if (!strcmp(http->method, "POST")) { } else if (!strcmp(http->method, "POST")) {
urlParsePostBody(url, http, buf, len); urlParsePostBody(url, http, buf, len);

59
shellinabox/launcher.c
View file

@ -273,33 +273,29 @@ static int read_string(int echo, const char *prompt, char **retstr) {
term_tmp.c_lflag &= ~ECHO; term_tmp.c_lflag &= ~ECHO;
} }
int nc; int nc;
for (;;) { tcsetattr(0, TCSAFLUSH, &term_tmp);
tcsetattr(0, TCSAFLUSH, &term_tmp); fprintf(stderr, "%s", prompt);
fprintf(stderr, "%s", prompt); char *line;
char *line; const int lineLength = 512;
const int lineLength = 512; check(line = calloc(1, lineLength));
check(line = calloc(1, lineLength)); nc = read(0, line, lineLength - 1);
nc = read(0, line, lineLength - 1); tcsetattr(0, TCSADRAIN, &term_before);
tcsetattr(0, TCSADRAIN, &term_before); if (!echo) {
if (!echo) { fprintf(stderr, "\n");
}
if (nc > 0) {
if (line[nc-1] == '\n') {
nc--;
} else if (echo) {
fprintf(stderr, "\n"); fprintf(stderr, "\n");
} }
if (nc > 0) { line[nc] = '\000';
if (line[nc-1] == '\n') { check(*retstr = line);
nc--; } else {
} else if (echo) { memset(line, 0, lineLength);
fprintf(stderr, "\n"); free(line);
} if (echo) {
line[nc] = '\000'; fprintf(stderr, "\n");
check(*retstr = line);
break;
} else {
memset(line, 0, lineLength);
free(line);
if (echo) {
fprintf(stderr, "\n");
}
break;
} }
} }
tcsetattr(0, TCSADRAIN, &term_before); tcsetattr(0, TCSADRAIN, &term_before);
@ -711,6 +707,7 @@ void closeAllFds(int *exceptFds, int num) {
// Close all file handles. If possible, scan through "/proc/self/fd" as // Close all file handles. If possible, scan through "/proc/self/fd" as
// that is faster than calling close() on all possible file handles. // that is faster than calling close() on all possible file handles.
int nullFd = open("/dev/null", O_RDWR); int nullFd = open("/dev/null", O_RDWR);
check(nullFd > 2);
DIR *dir = opendir("/proc/self/fd"); DIR *dir = opendir("/proc/self/fd");
if (dir == 0) { if (dir == 0) {
for (int i = sysconf(_SC_OPEN_MAX); --i > 0; ) { for (int i = sysconf(_SC_OPEN_MAX); --i > 0; ) {
@ -805,7 +802,7 @@ static int forkPty(int *pty, int useLogin, struct Utmp **utmp,
size_t length = 32; size_t length = 32;
char* path = NULL; char* path = NULL;
while (path == NULL) { while (path == NULL) {
path = malloc (length); check(path = malloc(length));
*path = 0; *path = 0;
if (ptsname_r (*pty, path, length)) { if (ptsname_r (*pty, path, length)) {
if (errno == ERANGE) { if (errno == ERANGE) {
@ -913,7 +910,7 @@ static int forkPty(int *pty, int useLogin, struct Utmp **utmp,
(*utmp)->utmpx.ut_pid = pid; (*utmp)->utmpx.ut_pid = pid;
#endif #endif
(*utmp)->pty = *pty; (*utmp)->pty = *pty;
fcntl(*pty, F_SETFL, O_NONBLOCK|O_RDWR); check(!fcntl(*pty, F_SETFL, O_NONBLOCK | O_RDWR));
NOINTR(close(slave)); NOINTR(close(slave));
return pid; return pid;
} }
@ -1449,7 +1446,6 @@ static void execService(int width ATTR_UNUSED, int height ATTR_UNUSED,
extern char **environ; extern char **environ;
environ = environment; environ = environment;
char *cmd = strdup(argv[0]);
char *slash = strrchr(argv[0], '/'); char *slash = strrchr(argv[0], '/');
if (slash) { if (slash) {
memmove(argv[0], slash + 1, strlen(slash)); memmove(argv[0], slash + 1, strlen(slash));
@ -1461,7 +1457,12 @@ static void execService(int width ATTR_UNUSED, int height ATTR_UNUSED,
argv[0][0] = '-'; argv[0][0] = '-';
argv[0][len + 1] = '\000'; argv[0][len + 1] = '\000';
} }
execvp(cmd, argv); char *cmd;
check(cmd = strdup(argv[0]));
if (execvp(cmd, argv) < 0) {
free(argv);
free(cmd);
}
} }
void setWindowSize(int pty, int width, int height) { void setWindowSize(int pty, int width, int height) {

2
shellinabox/privileges.c
View file

@ -103,7 +103,7 @@ static void removeGroupPrivileges(int showError) {
if (runAsGroup >= 0) { if (runAsGroup >= 0) {
uid_t ru, eu, su; uid_t ru, eu, su;
getresuid(&ru, &eu, &su); check(!getresuid(&ru, &eu, &su));
// Try to switch the user-provided group. // Try to switch the user-provided group.
if ((ru && runAsGroup != (int)rg) || if ((ru && runAsGroup != (int)rg) ||

24
shellinabox/shellinaboxd.c
View file

@ -503,6 +503,9 @@ static void serveStaticFile(HttpConnection *http, const char *contentType,
if (!memcmp(ptr, "[if ", 4)) { if (!memcmp(ptr, "[if ", 4)) {
char *bracket = memchr(ptr + 4, ']', eol - ptr - 4); char *bracket = memchr(ptr + 4, ']', eol - ptr - 4);
if (bracket != NULL && bracket > ptr + 4) { if (bracket != NULL && bracket > ptr + 4) {
if (tag != NULL) {
free(tag);
}
check(tag = malloc(bracket - ptr - 3)); check(tag = malloc(bracket - ptr - 3));
memcpy(tag, ptr + 4, bracket - ptr - 4); memcpy(tag, ptr + 4, bracket - ptr - 4);
tag[bracket - ptr - 4] = '\000'; tag[bracket - ptr - 4] = '\000';
@ -944,7 +947,7 @@ static void parseArgs(int argc, char * const argv[]) {
fatal("Only one certificate file handle can be provided"); fatal("Only one certificate file handle can be provided");
} }
if (!optarg || *optarg < '0' || *optarg > '9') { if (!optarg || *optarg < '0' || *optarg > '9') {
fatal("\"--cert-fd\" expects a valid file handle"); fatal("Option \"--cert-fd\" expects a valid file handle.");
} }
int tmpFd = strtoint(optarg, 3, INT_MAX); int tmpFd = strtoint(optarg, 3, INT_MAX);
certificateFd = dup(tmpFd); certificateFd = dup(tmpFd);
@ -956,7 +959,7 @@ static void parseArgs(int argc, char * const argv[]) {
// CSS // CSS
struct stat st; struct stat st;
if (!optarg || !*optarg || stat(optarg, &st) || !S_ISREG(st.st_mode)) { if (!optarg || !*optarg || stat(optarg, &st) || !S_ISREG(st.st_mode)) {
fatal("\"--css\" expects a file name"); fatal("Option \"--css\" expects a file name.");
} }
FILE *css = fopen(optarg, "r"); FILE *css = fopen(optarg, "r");
if (!css) { if (!css) {
@ -1003,6 +1006,9 @@ static void parseArgs(int argc, char * const argv[]) {
logSetLogLevel(verbosity); logSetLogLevel(verbosity);
} else if (!idx--) { } else if (!idx--) {
// Static file // Static file
if (!optarg || !*optarg) {
fatal("Option \"--static-file\" expects an argument.");
}
char *ptr, *path, *file; char *ptr, *path, *file;
if ((ptr = strchr(optarg, ':')) == NULL) { if ((ptr = strchr(optarg, ':')) == NULL) {
fatal("Syntax error in static-file definition \"%s\".", optarg); fatal("Syntax error in static-file definition \"%s\".", optarg);
@ -1021,11 +1027,14 @@ static void parseArgs(int argc, char * const argv[]) {
fatal("Duplicate --group option."); fatal("Duplicate --group option.");
} }
if (!optarg || !*optarg) { if (!optarg || !*optarg) {
fatal("\"--group\" expects a group name."); fatal("Option \"--group\" expects a group name.");
} }
runAsGroup = parseGroupArg(optarg, NULL); runAsGroup = parseGroupArg(optarg, NULL);
} else if (!idx--) { } else if (!idx--) {
// Linkify // Linkify
if (!optarg || !*optarg) {
fatal("Option \"--linkify\" expects an argument.");
}
if (!strcmp(optarg, "none")) { if (!strcmp(optarg, "none")) {
linkifyURLs = 0; linkifyURLs = 0;
} else if (!strcmp(optarg, "normal")) { } else if (!strcmp(optarg, "normal")) {
@ -1066,11 +1075,14 @@ static void parseArgs(int argc, char * const argv[]) {
fatal("Cannot specifiy a port for CGI operation"); fatal("Cannot specifiy a port for CGI operation");
} }
if (!optarg || *optarg < '0' || *optarg > '9') { if (!optarg || *optarg < '0' || *optarg > '9') {
fatal("\"--port\" expects a port number."); fatal("Option \"--port\" expects a port number.");
} }
port = strtoint(optarg, 1, 65535); port = strtoint(optarg, 1, 65535);
} else if (!idx--) { } else if (!idx--) {
// Service // Service
if (!optarg || !*optarg) {
fatal("Option \"--service\" expects an argument.");
}
struct Service *service; struct Service *service;
service = newService(optarg); service = newService(optarg);
if (getRefFromHashMap(serviceTable, service->path)) { if (getRefFromHashMap(serviceTable, service->path)) {
@ -1103,13 +1115,13 @@ static void parseArgs(int argc, char * const argv[]) {
fatal("Duplicate --user option."); fatal("Duplicate --user option.");
} }
if (!optarg || !*optarg) { if (!optarg || !*optarg) {
fatal("\"--user\" expects a user name."); fatal("Option \"--user\" expects a user name.");
} }
runAsUser = parseUserArg(optarg, NULL); runAsUser = parseUserArg(optarg, NULL);
} else if (!idx--) { } else if (!idx--) {
// User CSS // User CSS
if (!optarg || !*optarg) { if (!optarg || !*optarg) {
fatal("\"--user-css\" expects a list of styles sheets and labels"); fatal("Option \"--user-css\" expects a list of styles sheets and labels");
} }
parseUserCSS(&userCSSList, optarg); parseUserCSS(&userCSSList, optarg);
} else if (!idx--) { } else if (!idx--) {