Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Catch other exceptions from readDataset to handle file open failures #124

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
10 changes: 6 additions & 4 deletions api/bag.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
#include "bag_vrrefinements.h"
#include "bag_vrrefinementsdescriptor.h"
#include "bag_vrtrackinglist.h"
#include <array>
#include <cstdio>

#ifdef _MSC_VER
#pragma warning(push)
Expand All @@ -32,14 +34,13 @@
#include <sstream>
#include <string>
#include <string.h>
#include "signal_hook.h"

#ifdef _MSC_VER
#pragma warning(pop)
#endif


namespace {

//! Convert a BAG::CompoundDataType (C++) into a BagCompoundDataType (C).
/*!
\param field
Expand Down Expand Up @@ -111,8 +112,6 @@ BAG::CompoundDataType getValue(
}
}

} // namespace

//! Open the specified BAG.
/*!
\param handle
Expand All @@ -134,6 +133,8 @@ BagError bagFileOpen(
BAG_OPEN_MODE accessMode,
const char* fileName)
{
auto bag_hook = BagAbortHook();

if (!handle)
return BAG_INVALID_BAG_HANDLE;

Expand Down Expand Up @@ -169,6 +170,7 @@ BagError bagFileOpen(
BagError bagFileClose(
BagHandle* handle)
{
auto bag_hook = BagAbortHook();
if (!handle)
return BAG_INVALID_BAG_HANDLE;

Expand Down
11 changes: 11 additions & 0 deletions api/bag_dataset.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,17 @@ std::shared_ptr<Dataset> Dataset::open(
{
std::cerr << "\nUnable to open BAG file: " << fileName << " due to error: " << fileExcept.getCDetailMsg();
return nullptr;
} catch (H5::GroupIException &groupExcept) {
std::cerr << "\nAn group exception occurred opening the dataset. Error was in "
<< groupExcept.getFuncName()
<< ". A detailed message follows:"
<< std::endl << groupExcept.getDetailMsg() << std::endl;
} catch (H5::Exception &otherException)
{
std::cerr << "\nAn unknown exception occurred opening the dataset. Error was in "
<< otherException.getFuncName()
<< ". A detailed message follows:"
<< std::endl << otherException.getDetailMsg() << std::endl;
}

return pDataset;
Expand Down
235 changes: 235 additions & 0 deletions api/signal_hook.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,235 @@
#ifndef BAG_SIGNAL_HOOK
#define BAG_SIGNAL_HOOK

#include <iostream>
#include <ostream>
#include <string>

// from https://man7.org/linux/man-pages/man2/sigaction.2.html
// and https://man7.org/linux/man-pages/man7/feature_test_macros.7.html
#ifdef _POSIX_C_SOURCE

#include <string.h>
#include <signal.h>
#include <array>
#include <csignal>
#include <cstdio>
#include <unistd.h>

static void (*cleanup_functions[128])(int);
static int cleanup_functions_count = 0;

// Returns 0 on success, 1 if too many functions have been set
// This is not thread safe, only call it from a single thread
int add_cleanup_function(void (*f)(int)) {
if (cleanup_functions_count >= 128) {
return 1;
}

cleanup_functions[cleanup_functions_count] = f;
cleanup_functions_count++;

return 0;
}


static void bag_handler(int signal);

struct BagAbortHook;

static BagAbortHook* current_hook;

static int bag_string_length(const char* string) {
int length = 0;
while(true) {
if (string[length] == 0) {
break;
}
length++;
}

return length;
}

struct BagAbortHook {
BagAbortHook* previous;
//void (*old_handler[255])(int);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove commented out code

//void (*current_handler[255])(int);
struct sigaction old_handlers[255];
Copy link
Collaborator

Choose a reason for hiding this comment

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

The size of these handler arrays should be a constant defined by a macro (i.e. #define somewhere).

//void (*old_handlers[255])(int);

// returns after done printing the exit message
void print_error_msg(int signal) {
// if the handlers are the same, don't call "bag handler" twice
char* signal_name;

#define TERM_STRING "a critical error occurred within BAG or a dependency (probably libxml or HDF5), exiting to prevent corruption or unexpected behavior\n"\
Copy link
Collaborator

Choose a reason for hiding this comment

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

probably shouldn't put any newlines in this string since we don't know where it's going to ultimately be written to.

"the signal that caused this error was: "

// I can't figure out how to tell if sigabbrev_np exists so I'll
// just print the ones it probably is
const char* signal_abbrev = "<unknown>";

if(signal == SIGSEGV) {
signal_abbrev = "SIGSEGV";
} else if(signal == SIGABRT) {
signal_abbrev = "SIGABRT";
} else if(signal == SIGFPE) {
signal_abbrev = "SIGFPE";
} else if(signal == SIGKILL) {
signal_abbrev = "SIGKILL";
} else if(signal == SIGILL) {
signal_abbrev = "SIGILL";
} else if(signal == SIGBUS) {
signal_abbrev = "SIGBUS";
}

/*
#if defined(__USE_GNU) && defined(_GNU_SOURCE)
const char* signal_abbrev = sigabbrev_np(signal);
#else
const char* signal_abbrev = "<unknown>";
#endif
if (!signal_abbrev) {
signal_abbrev = "<bad signal number>";
}
*/

// I think this works, it seems to? write needs an fd
int stderr_fd = stderr->_fileno;

// strlen is not async signal safe, so it can't be used here, but bag_string_length works instead
write(stderr_fd, TERM_STRING, bag_string_length(TERM_STRING));
write(stderr_fd, signal_abbrev, bag_string_length(signal_abbrev));
write(stderr_fd, "\n", 1);
}

void call_cleanup_functions(int signal) {
//std::cout << "calling cleanup functions" << std::endl;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove commented out code.

for(int i = 0; i < cleanup_functions_count; i++) {
//std::cout << "calling cleanup function:" << i << std::endl;
cleanup_functions[i](signal);
}
}

void call_previous_handler(int signal) {
if (this->old_handlers[signal].sa_handler != nullptr && this->old_handlers[signal].sa_handler != bag_handler) {
this->old_handlers[signal].sa_handler(signal);
}
}

void handle(int signal) {
this->print_error_msg(signal);

// if anyone set a cleanup function, call those
this->call_cleanup_functions(signal);

// if there was a handler before this, that isn't the BAG handler,
// call that one

Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove blank line

this->call_previous_handler(signal);

// if nothing has exited in the previous handlers, exit here
//std::cout << "exiting" << std::endl;
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove commented out code

_exit(-1);
}

std::array<int, 6> handled_signals() {
return { SIGABRT, SIGILL, SIGBUS, SIGFPE, SIGSEGV, SIGKILL };
}

BagAbortHook() {
std::cout << "made hook" << std::endl;
// set these all to null so that when restoring them
// we know which ones were added
for (int i = 0; i < 255; i++) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

use a uint8_t for looks like this?

this->old_handlers[i].sa_handler = nullptr;
}

struct sigaction old_action;
for (auto sn : this->handled_signals()) {
//std::cout << "setting action for signal " << sn << std::endl;
struct sigaction new_action;
new_action.sa_flags = 0;
new_action.sa_handler = bag_handler;

// I was going to use https://en.cppreference.com/w/c/program/signal here
// but I needed to get what the old action was so that I could
// put the original handler back
// https://linux.die.net/man/2/sigaction seems to give me what it used to be
// in old action, but I'm not sure if I need to do anything special with the
// other things in the sigaction struct?
//
// Actually, I think it does support it but apparently signal is
// pretty outdated: https://stackoverflow.com/questions/231912/what-is-the-difference-between-sigaction-and-signal
auto r = sigaction(sn, &new_action, &old_action);
//auto old_handler = signal(sn, bag_handler);

/*if (old_handler == SIG_ERR) {
std::cout << "got SIG_ERR setting signal handler?" << std::endl;
} else if (old_handler == SIG_DFL) {
std::cout << "old handler was DFL" << std::endl;
} else if (old_handler == SIG_IGN) {
std::cout << "old handler was IGN" << std::endl;
}*/

//std::cout << "setting action returned:" << old_handler << std::endl;

this->old_handlers[sn] = old_action;
}
this->previous = current_hook;
current_hook = this;
}

~BagAbortHook() {
std::cout << "resetting to original handlers" << std::endl;
// reset the handlers back to what they were
for (auto sn : this->handled_signals()) {
struct sigaction old_action = this->old_handlers[sn];
//auto old_action = this->old_handlers[sn];
//signal(sn, old_action);
sigaction(sn, &old_action, nullptr);
}

current_hook = this->previous;
}
};

static void bag_handler(int signal) {
//std::cout << "handler" << std::endl;
if (current_hook) {
current_hook->handle(signal);
} else {
// something broke I think?
// I don't know if there's a way to deal with this
// so I'll just exit here
_exit(-2);
}
}

#else // this isn't linux with sigaction, so make a BagAbortHook that does nothing

static bool bag_warning_printed = false;

static void print_disabled_warning() {
if (!bag_warning_printed) {
bag_warning_printed = true;
std::cout << "WARNING: BagAbortHook and add_cleanup_function are disabled because _POSIX_C_SOURCE does not say that sigaction is available" << std::endl;
}
}

int add_cleanup_function(void (*)(int)) {
print_disabled_warning();
return 0;
}


struct BagAbortHook {
BagAbortHook() {
print_disabled_warning();
}
};

#endif

#endif
5 changes: 3 additions & 2 deletions examples/driver.cpp
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@

#include <string>
#include <cstring>
#include <iostream>
#include <fstream>
#include <string>
#include <ios>
#include <vector>
#include <stdint.h>
Expand Down Expand Up @@ -40,4 +41,4 @@ int main(int argc, char** argv)//argv, which is an array of pointers to strings
//strcpy(filename_cstr, filename.c_str());

LLVMFuzzerTestOneInputByFile(filename.c_str());
}
}
33 changes: 33 additions & 0 deletions examples/test_signal_hook.cpp
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please integrate this into the C++ test suite so that it is automatically run in CI.

Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
#include "../api/signal_hook.h"
#include <csignal>
#include <cstdlib>
#include <iostream>
#include <ostream>

void cleanuper(int signal) {
std::cout << "cleanuper" << std::endl;
}

void existing_handler(int signal) {
std::cout << "existing handler" << std::endl;
}

int main(int argc, char** argv) {
signal(SIGSEGV, existing_handler);

BagAbortHook hook;

add_cleanup_function(cleanuper);

std::cout << "signal going to happen" << std::endl;

// this causes the segfault, but not sure if compiler might do something else?
// this is just a test, so could maybe make it do sigabrt instead?
volatile int* p = nullptr;

int v = *p;

std::cout << "v is:" << v << std::endl;

std::cout << "this shouldn't print" << std::endl;
}
Loading