Skip to content
This repository has been archived by the owner on Jan 26, 2019. It is now read-only.

Improve error messages #84

Merged
merged 3 commits into from
Nov 22, 2017
Merged

Improve error messages #84

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ea2b679
Use calloc everywhere
oschwald Nov 21, 2017
8fbca6a
Improve error messages to include reasons
oschwald Nov 21, 2017
17a53e2
Remove redundant memset
oschwald Nov 22, 2017
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
2 changes: 2 additions & 0 deletions ChangeLog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
GeoIP Update Change Log
=======================

* Improve the error checking and display the underlying reason for the
error when possible. Reported by Jonathan Kosgei. GitHub #82.
* Document that the `LockFile` is not removed from the filesystem after
a successful exit from the program. GitHub issue #79.

Expand Down
6 changes: 4 additions & 2 deletions bin/edition_s.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@

#include "geoipupdate.h"
#include <errno.h>
#include <stdlib.h>
#include <string.h>

Expand Down Expand Up @@ -32,10 +33,11 @@ void edition_insert_once(geoipupdate_s *gu, const char *edition_id) {
}

edition_s *edition_new(const char *edition_id) {
edition_s *p = xmalloc(sizeof(edition_s));
edition_s *p = xcalloc(1, sizeof(edition_s));
p->edition_id = strdup(edition_id);
exit_if(NULL == p->edition_id,
"Unable to allocate memory for edition ID.\n");
"Unable to allocate memory for edition ID: %s\n",
strerror(errno));
p->next = NULL;
return p;
}
Expand Down
136 changes: 86 additions & 50 deletions bin/geoipupdate.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ typedef struct {
} in_mem_s;

static void xasprintf(char **, const char *, ...);
static void *xcalloc(size_t, size_t);
static void *xrealloc(void *, size_t);
static void usage(void);
static int parse_opts(geoipupdate_s *, int, char *const[]);
Expand Down Expand Up @@ -74,7 +73,7 @@ static void xasprintf(char **ptr, const char *fmt, ...) {
va_start(ap, fmt);
int rc = vasprintf(ptr, fmt, ap);
va_end(ap);
exit_if(rc == -1, "Out of memory\n");
exit_if(rc == -1, "Error calling vasprintf: %s\n", strerror(errno));
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, will vasprintf() actually set errno? Normally I'd go by what the man page says and I don't see it saying it does for this function.

Copy link
Member Author

Choose a reason for hiding this comment

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

All of the POSIX printf functions set errno:

http://pubs.opengroup.org/onlinepubs/9699919799/functions/fprintf.html

For vasprintf(), I checked both FreeBSD and glibc. Both set it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Much better docs than the man page. I'll have to remember to look there.

I don't think this one is POSIX, is it? It sounds like it's okay to expect it be set anyway though.

}

void say_if(int expr, const char *fmt, ...) {
Expand All @@ -87,21 +86,15 @@ void say_if(int expr, const char *fmt, ...) {
va_end(ap);
}

static void *xcalloc(size_t nmemb, size_t size) {
void *xcalloc(size_t nmemb, size_t size) {
void *ptr = calloc(nmemb, size);
exit_if(!ptr, "Out of memory\n");
return ptr;
}

void *xmalloc(size_t size) {
void *ptr = malloc(size);
exit_if(!ptr, "Out of memory\n");
exit_if(!ptr, "Error allocating memory: %s\n", strerror(errno));
Copy link
Contributor

Choose a reason for hiding this comment

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

If we were to be very pedantic, I'm not sure we can guarantee calloc() will set errno. C99 doesn't require it, at least. Maybe this is ugly, but in the past I've just done strerror(ENOMEM) for such cases. It looks like there are some existing cases of this as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

In all modern Unixes, this should be set properly. I double checked on Windows and it appears it gets set there too.

From the calloc man page:

   SUSv2 requires malloc(), calloc(), and realloc() to set errno to ENOMEM upon failure.  Glibc assumes that this is done (and the glibc versions
   of these routines do this); if you use a private malloc implementation that does not set errno, then certain library routines may fail without
   having a reason in errno.

Is there a particular implementation that you are concerned about?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I mainly went by what you pasted and it not being specified in the standard.

Copy link
Contributor

@horgh horgh Nov 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

We have the same situation where POSIX says it should be set, so that's another point in favour of using errno!

return ptr;
}

static void *xrealloc(void *ptr, size_t size) {
void *mem = realloc(ptr, size);
exit_if(mem == NULL, "Out of memory\n");
exit_if(mem == NULL, "Error reallocating memory: %s\n", strerror(errno));
Copy link
Contributor

Choose a reason for hiding this comment

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

My comment about calloc() applies to realloc()

return mem;
}

Expand Down Expand Up @@ -133,9 +126,10 @@ static int parse_opts(geoipupdate_s *gu, int argc, char *const argv[]) {
case 'd':
free(gu->database_dir);
gu->database_dir = strdup(optarg);
exit_if(
NULL == gu->database_dir,
"Unable to allocate memory for database directory path.");
exit_if(NULL == gu->database_dir,
"Unable to allocate memory for database directory "
"path: %s\n",
strerror(errno));

// The database directory in the config file is ignored if we
// use -d
Expand All @@ -145,7 +139,8 @@ static int parse_opts(geoipupdate_s *gu, int argc, char *const argv[]) {
free(gu->license_file);
gu->license_file = strdup(optarg);
exit_if(NULL == gu->license_file,
"Unable to allocate memory for license file path.\n");
"Unable to allocate memory for license file path: %s\n",
strerror(errno));
break;
case 'h':
default:
Expand Down Expand Up @@ -176,8 +171,9 @@ int main(int argc, char *const argv[]) {
parse_opts(gu, argc, argv);
if (parse_license_file(gu)) {
exit_unless(stat(gu->database_dir, &st) == 0,
"%s does not exist\n",
gu->database_dir);
"%s does not exist: %s\n",
gu->database_dir,
strerror(errno));
exit_unless(S_ISDIR(st.st_mode),
"%s is not a directory\n",
gu->database_dir);
Expand All @@ -187,8 +183,9 @@ int main(int argc, char *const argv[]) {
// open the file, and avoid potential race issues where permissions
// change between now and then.
exit_unless(access(gu->database_dir, W_OK) == 0,
"%s is not writable\n",
gu->database_dir);
"%s is not writable: %s\n",
gu->database_dir,
strerror(errno));

if (acquire_run_lock(gu) != 0) {
geoipupdate_s_delete(gu);
Expand Down Expand Up @@ -221,12 +218,15 @@ static ssize_t my_getline(char **linep, size_t *linecapp, FILE *stream) {
static int parse_license_file(geoipupdate_s *up) {
say_if(up->verbose, "%s\n", PACKAGE_STRING);
FILE *fh = fopen(up->license_file, "rb");
exit_unless(!!fh, "Can't open license file %s\n", up->license_file);
exit_unless(!!fh,
"Can't open license file %s: %s\n",
up->license_file,
strerror(errno));
say_if(up->verbose, "Opened License file %s\n", up->license_file);

const char *sep = " \t\r\n";
size_t bsize = 1024;
char *buffer = (char *)xmalloc(bsize);
char *buffer = (char *)xcalloc(bsize, sizeof(char));
ssize_t read_bytes;
while ((read_bytes = my_getline(&buffer, &bsize, fh)) != -1) {
size_t idx = strspn(buffer, sep);
Expand Down Expand Up @@ -273,7 +273,8 @@ static int parse_license_file(geoipupdate_s *up) {
free(up->proto);
up->proto = strdup(p);
exit_if(NULL == up->proto,
"Unable to allocate memory for request protocol.\n");
"Unable to allocate memory for request protocol: %s\n",
strerror(errno));
} else if (!strcmp(p, "SkipHostnameVerification")) {
p = strtok_r(NULL, sep, &last);
exit_if(NULL == p ||
Expand All @@ -286,7 +287,8 @@ static int parse_license_file(geoipupdate_s *up) {
free(up->host);
up->host = strdup(p);
exit_if(NULL == up->host,
"Unable to allocate memory for update host.\n");
"Unable to allocate memory for update host: %s\n",
strerror(errno));
} else if (!strcmp(p, "DatabaseDirectory")) {
if (!up->do_not_overwrite_database_directory) {
p = strtok_r(NULL, sep, &last);
Expand All @@ -295,23 +297,26 @@ static int parse_license_file(geoipupdate_s *up) {
up->database_dir = strdup(p);
exit_if(NULL == up->database_dir,
"Unable to allocate memory for database directory "
"path.\n");
"path: %s\n",
strerror(errno));
}
} else if (!strcmp(p, "Proxy")) {
p = strtok_r(NULL, sep, &last);
exit_if(NULL == p, "Proxy must be defined 1.2.3.4:12345\n");
free(up->proxy);
up->proxy = strdup(p);
exit_if(NULL == up->proxy,
"Unable to allocate memory for proxy host.\n");
"Unable to allocate memory for proxy host: %s\n",
strerror(errno));
} else if (!strcmp(p, "ProxyUserPassword")) {
p = strtok_r(NULL, sep, &last);
exit_if(NULL == p,
"ProxyUserPassword must be defined xyz:abc\n");
free(up->proxy_user_password);
up->proxy_user_password = strdup(p);
exit_if(NULL == up->proxy_user_password,
"Unable to allocate memory for proxy credentials.\n");
"Unable to allocate memory for proxy credentials: %s\n",
strerror(errno));
} else if (!strcmp(p, "LockFile")) {
p = strtok_r(NULL, sep, &last);
exit_if(NULL == p, "LockFile must be a file path\n");
Expand All @@ -320,7 +325,8 @@ static int parse_license_file(geoipupdate_s *up) {
free(up->lock_file);
up->lock_file = strdup(p);
exit_if(NULL == up->lock_file,
"Out of memory allocating LockFile string\n");
"Unable to allocate memory for LockFile string: %s\n",
strerror(errno));
} else {
say_if(up->verbose, "Skip unknown directive: %s\n", p);
}
Expand Down Expand Up @@ -463,19 +469,24 @@ static int md5hex(const char *fname, char *hex_digest) {
}

struct stat st;
exit_unless(stat(fname, &st) == 0 && S_ISREG(st.st_mode),
"%s is not a file\n",
fname);
exit_unless(stat(fname, &st) == 0,
"Unable to stat %s: %s\n",
fname,
strerror(errno));
exit_unless(S_ISREG(st.st_mode), "%s is not a file\n", fname);

md5_init(&context);
while ((len = fread(buffer, 1, bsize, fh)) > 0) {
md5_write(&context, buffer, len);
}
exit_if(ferror(fh), "Unable to read %s: %s\n", fname, strerror(errno));
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this is another case I'm not sure we get errno set.

Copy link
Member Author

Choose a reason for hiding this comment

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

fread appears to set errno: http://pubs.opengroup.org/onlinepubs/009695399/functions/fread.html

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh good.


md5_final(&context);
memcpy(digest, context.buf, 16);
exit_if(-1 == fclose(fh), "Error closing stream: %s", strerror(errno));
for (int i = 0; i < 16; i++) {
snprintf(&hex_digest[2 * i], 3, "%02x", digest[i]);
int c = snprintf(&hex_digest[2 * i], 3, "%02x", digest[i]);
exit_if(c < 0, "Unable to write digest: %s\n", strerror(errno));
Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise, does snprintf() set errno?

Copy link
Member Author

Choose a reason for hiding this comment

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

See above.

}
return 1;
}
Expand Down Expand Up @@ -632,8 +643,8 @@ static size_t mem_cb(void *contents, size_t size, size_t nmemb, void *userp) {
}

static in_mem_s *in_mem_s_new(void) {
in_mem_s *mem = (in_mem_s *)xmalloc(sizeof(in_mem_s));
mem->ptr = (char *)xcalloc(1, 1);
in_mem_s *mem = (in_mem_s *)xcalloc(1, sizeof(in_mem_s));
mem->ptr = (char *)xcalloc(1, sizeof(char));
mem->size = 0;
return mem;
}
Expand Down Expand Up @@ -875,10 +886,11 @@ static int gunzip_and_replace(geoipupdate_s const *const gu,
say_if(gu->verbose, "Uncompress file %s to %s\n", gzipfile, file_path_test);

gzFile gz_fh = gzopen(gzipfile, "rb");
exit_if(gz_fh == NULL, "Can't open %s\n", gzipfile);
exit_if(gz_fh == NULL, "Can't open %s: %s\n", gzipfile, strerror(errno));

FILE *fhw = fopen(file_path_test, "wb");
exit_if(fhw == NULL, "Can't open %s\n", file_path_test);
exit_if(
fhw == NULL, "Can't open %s: %s\n", file_path_test, strerror(errno));

size_t const bsize = 8192;
char *const buffer = calloc(bsize, sizeof(char));
Expand All @@ -892,30 +904,54 @@ static int gunzip_and_replace(geoipupdate_s const *const gu,

for (;;) {
int amt = gzread(gz_fh, buffer, bsize);
if (amt == 0) {
// EOF
break;
if (amt <= 0) {
if (gzeof(gz_fh)) {
// EOF
break;
}
int gzerr = 0;
const char *msg = gzerror(gz_fh, &gzerr);
if (gzerr == Z_ERRNO) {
fprintf(stderr,
"Unable to read %s: %s\n",
gzipfile,
strerror(errno));
} else {
fprintf(stderr, "Unable to decompress %s: %s\n", gzipfile, msg);
}
exit(1);
}
exit_if(amt == -1, "Gzip read error while reading from %s\n", gzipfile);
exit_unless(fwrite(buffer, 1, amt, fhw) == (size_t)amt,
"Gzip write error\n");
"Unable to write to %s: %s\n",
file_path_test,
strerror(errno));
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think fwrite() will set errno

}
exit_if(-1 == fclose(fhw), "Error closing stream: %s\n", strerror(errno));
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, the docs say fclose() does set errno. At least there is consistency!

if (gzclose(gz_fh) != Z_OK) {
int gzerr = 0;
const char *msg = gzerror(gz_fh, &gzerr);
if (gzerr == Z_ERRNO) {
msg = strerror(errno);
}
fprintf(stderr, "Unable to close %s: %s\n", gzipfile, msg);
exit(1);
}
exit_if(-1 == fclose(fhw), "Error closing stream: %s", strerror(errno));
exit_if(gzclose(gz_fh) != Z_OK,
"Gzip read error while closing from %s\n",
gzipfile);
free(buffer);

char actual_md5[33] = {0};
md5hex(file_path_test, actual_md5);
exit_if(strncasecmp(actual_md5, expected_file_md5, 32),
"MD5 of new database (%s) does not match expected MD5 (%s)",
"MD5 of new database (%s) does not match expected MD5 (%s)\n",
actual_md5,
expected_file_md5);

say_if(gu->verbose, "Rename %s to %s\n", file_path_test, geoip_filename);
int err = rename(file_path_test, geoip_filename);
exit_if(err, "Rename %s to %s failed\n", file_path_test, geoip_filename);
exit_if(err,
"Rename %s to %s failed: %s\n",
file_path_test,
geoip_filename,
strerror(errno));

if (gu->preserve_file_times && filetime > 0) {
struct utimbuf utb;
Expand All @@ -931,15 +967,15 @@ static int gunzip_and_replace(geoipupdate_s const *const gu,
// fsync directory to ensure the rename is durable
int dirfd = open(gu->database_dir, O_DIRECTORY);
exit_if(
-1 == dirfd, "Error opening database directory: %s", strerror(errno));
-1 == dirfd, "Error opening database directory: %s\n", strerror(errno));
exit_if(-1 == fsync(dirfd),
"Error syncing database directory: %s",
"Error syncing database directory: %s\n",
strerror(errno));
exit_if(-1 == close(dirfd),
"Error closing database directory: %s",
"Error closing database directory: %s\n",
strerror(errno));
exit_if(-1 == unlink(gzipfile),
"Error unlinking %s: %s",
"Error unlinking %s: %s\n",
gzipfile,
strerror(errno));

Expand Down
2 changes: 1 addition & 1 deletion bin/geoipupdate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ void edition_delete(edition_s *p);

void exit_unless(int expr, const char *fmt, ...);
void say_if(int expr, const char *fmt, ...);
void *xmalloc(size_t size);
void *xcalloc(size_t, size_t);

#define NO_ACCOUNT_ID (-1)
#define GEOIP_USERAGENT "geoipupdate/" VERSION
Expand Down
Loading