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

2
libhttp/server.c
View file

@ -338,7 +338,7 @@ void destroyServer(struct Server *server) {
if (server) {
if (server->serverFd >= 0) {
info("Shutting down server");
close(server->serverFd);
NOINTR(close(server->serverFd));
}
for (int i = 0; i < server->numConnections; i++) {
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)
static void sslGenerateCertificate(const char *certificate,
const char *serverName) {
debug("Auto-generating missing certificate \"%s\" for \"%s\"",
certificate, serverName);
debug("Auto-generating missing certificate \"%s\" for \"%s\"",
certificate, serverName);
pid_t pid = fork();
pid_t pid = fork();
if (pid == -1) {
warn("Failed to generate self-signed certificate \"%s\"", certificate);
} else if (pid == 0) {
int fd = NOINTR(open("/dev/null", O_RDONLY));
int fd = NOINTR(open("/dev/null", O_RDONLY));
check(fd != -1);
check(NOINTR(dup2(fd, STDERR_FILENO)) == STDERR_FILENO);
check(NOINTR(close(fd)) == 0);
fd = NOINTR(open("/dev/null", O_WRONLY));
fd = NOINTR(open("/dev/null", O_WRONLY));
check(fd != -1);
check(NOINTR(dup2(fd, STDIN_FILENO)) == STDIN_FILENO);
check(NOINTR(close(fd)) == 0);
umask(077);
check(setenv("PATH", "/usr/bin:/usr/sbin", 1) == 0);
execlp("openssl", "openssl", "req", "-x509", "-nodes", "-days", "7300",
"-newkey", "rsa:2048", "-keyout", certificate, "-out", certificate,
"-subj", stringPrintf(NULL, "/CN=%s/", serverName),
(char *)NULL);
check(0);
char *subject;
check(subject = stringPrintf(NULL, "/CN=%s/", serverName));
if (execlp("openssl", "openssl", "req", "-x509", "-nodes", "-days", "7300",
"-newkey", "rsa:2048", "-keyout", certificate, "-out", certificate,
"-subj", subject, (char *)NULL) < 0) {
warn("Failed to generate self-signed certificate \"%s\"", certificate);
free(subject);
}
} else {
int status;
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);
}
}
}

1
libhttp/url.c
View file

@ -346,6 +346,7 @@ void initURL(struct URL *url, const struct HttpConnection *http,
url->url = NULL;
initHashMap(&url->args, urlDestroyHashMapEntry, NULL);
if (!strcmp(http->method, "GET")) {
check(url->query);
urlParseQueryString(&url->args, url->query, strlen(url->query));
} else if (!strcmp(http->method, "POST")) {
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;
}
int nc;
for (;;) {
tcsetattr(0, TCSAFLUSH, &term_tmp);
fprintf(stderr, "%s", prompt);
char *line;
const int lineLength = 512;
check(line = calloc(1, lineLength));
nc = read(0, line, lineLength - 1);
tcsetattr(0, TCSADRAIN, &term_before);
if (!echo) {
tcsetattr(0, TCSAFLUSH, &term_tmp);
fprintf(stderr, "%s", prompt);
char *line;
const int lineLength = 512;
check(line = calloc(1, lineLength));
nc = read(0, line, lineLength - 1);
tcsetattr(0, TCSADRAIN, &term_before);
if (!echo) {
fprintf(stderr, "\n");
}
if (nc > 0) {
if (line[nc-1] == '\n') {
nc--;
} else if (echo) {
fprintf(stderr, "\n");
}
if (nc > 0) {
if (line[nc-1] == '\n') {
nc--;
} else if (echo) {
fprintf(stderr, "\n");
}
line[nc] = '\000';
check(*retstr = line);
break;
} else {
memset(line, 0, lineLength);
free(line);
if (echo) {
fprintf(stderr, "\n");
}
break;
line[nc] = '\000';
check(*retstr = line);
} else {
memset(line, 0, lineLength);
free(line);
if (echo) {
fprintf(stderr, "\n");
}
}
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
// that is faster than calling close() on all possible file handles.
int nullFd = open("/dev/null", O_RDWR);
check(nullFd > 2);
DIR *dir = opendir("/proc/self/fd");
if (dir == 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;
char* path = NULL;
while (path == NULL) {
path = malloc (length);
check(path = malloc(length));
*path = 0;
if (ptsname_r (*pty, path, length)) {
if (errno == ERANGE) {
@ -913,7 +910,7 @@ static int forkPty(int *pty, int useLogin, struct Utmp **utmp,
(*utmp)->utmpx.ut_pid = pid;
#endif
(*utmp)->pty = *pty;
fcntl(*pty, F_SETFL, O_NONBLOCK|O_RDWR);
check(!fcntl(*pty, F_SETFL, O_NONBLOCK | O_RDWR));
NOINTR(close(slave));
return pid;
}
@ -1449,7 +1446,6 @@ static void execService(int width ATTR_UNUSED, int height ATTR_UNUSED,
extern char **environ;
environ = environment;
char *cmd = strdup(argv[0]);
char *slash = strrchr(argv[0], '/');
if (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][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) {

2
shellinabox/privileges.c
View file

@ -103,7 +103,7 @@ static void removeGroupPrivileges(int showError) {
if (runAsGroup >= 0) {
uid_t ru, eu, su;
getresuid(&ru, &eu, &su);
check(!getresuid(&ru, &eu, &su));
// Try to switch the user-provided group.
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)) {
char *bracket = memchr(ptr + 4, ']', eol - ptr - 4);
if (bracket != NULL && bracket > ptr + 4) {
if (tag != NULL) {
free(tag);
}
check(tag = malloc(bracket - ptr - 3));
memcpy(tag, ptr + 4, bracket - ptr - 4);
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");
}
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);
certificateFd = dup(tmpFd);
@ -956,7 +959,7 @@ static void parseArgs(int argc, char * const argv[]) {
// CSS
struct stat st;
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");
if (!css) {
@ -1003,6 +1006,9 @@ static void parseArgs(int argc, char * const argv[]) {
logSetLogLevel(verbosity);
} else if (!idx--) {
// Static file
if (!optarg || !*optarg) {
fatal("Option \"--static-file\" expects an argument.");
}
char *ptr, *path, *file;
if ((ptr = strchr(optarg, ':')) == NULL) {
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.");
}
if (!optarg || !*optarg) {
fatal("\"--group\" expects a group name.");
fatal("Option \"--group\" expects a group name.");
}
runAsGroup = parseGroupArg(optarg, NULL);
} else if (!idx--) {
// Linkify
if (!optarg || !*optarg) {
fatal("Option \"--linkify\" expects an argument.");
}
if (!strcmp(optarg, "none")) {
linkifyURLs = 0;
} 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");
}
if (!optarg || *optarg < '0' || *optarg > '9') {
fatal("\"--port\" expects a port number.");
fatal("Option \"--port\" expects a port number.");
}
port = strtoint(optarg, 1, 65535);
} else if (!idx--) {
// Service
if (!optarg || !*optarg) {
fatal("Option \"--service\" expects an argument.");
}
struct Service *service;
service = newService(optarg);
if (getRefFromHashMap(serviceTable, service->path)) {
@ -1103,13 +1115,13 @@ static void parseArgs(int argc, char * const argv[]) {
fatal("Duplicate --user option.");
}
if (!optarg || !*optarg) {
fatal("\"--user\" expects a user name.");
fatal("Option \"--user\" expects a user name.");
}
runAsUser = parseUserArg(optarg, NULL);
} else if (!idx--) {
// User CSS
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);
} else if (!idx--) {