diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml index 3b81d10e..9f1f46d7 100644 --- a/.github/workflows/unit-tests.yml +++ b/.github/workflows/unit-tests.yml @@ -25,6 +25,9 @@ jobs: - irods: "4.2.7" server_image: "wsinpg/ub-18.04-irods-4.2.10:latest" experimental: true + - irods: "4.2.7" + server_image: "wsinpg/ub-18.04-irods-4.2.11:latest" + experimental: true - irods: "4.2.10" server_image: "wsinpg/ub-16.04-irods-4.2.7:latest" @@ -32,10 +35,28 @@ jobs: - irods: "4.2.10" server_image: "wsinpg/ub-18.04-irods-4.2.10:latest" experimental: false + - irods: "4.2.10" + server_image: "wsinpg/ub-18.04-irods-4.2.11:latest" + experimental: true + + - irods: "4.2.11" + server_image: "wsinpg/ub-16.04-irods-4.2.7:latest" + experimental: true + - irods: "4.2.11" + server_image: "wsinpg/ub-18.04-irods-4.2.10:latest" + experimental: true + - irods: "4.2.11" + server_image: "wsinpg/ub-18.04-irods-4.2.11:latest" + experimental: false + services: irods: image: ${{ matrix.server_image }} + options: --name irods-server # A consistent container name is necessary + # to allow creation of bad replicas. + # 'irods-server' is the default container + # name in the bad replica script ports: - 1247:1247 - 20000-20199:20000-20199 diff --git a/ChangeLog b/ChangeLog index 76f5876c..1b2039df 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,18 @@ [Upcoming] + [3.3.0] + + Fix ability to list the checksum of an object with a bad replica. + + Add iRODS 4.2.11 to test matrix. + + Improve signal handling so that iRODS connections are closed + before exiting. + + Fix a segfault caused by irods error values being returned as if + they were the number of bytes read when a user did not have + permission to get an object. + [3.2.0] Fix a segfault when the file specified by the -f/--file option was diff --git a/src/Makefile.am b/src/Makefile.am index eb3a69f1..63f6886b 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -27,6 +27,7 @@ libbaton_include_HEADERS = baton.h \ operations.h \ query.h \ read.h \ + signal_handler.h \ utilities.h \ write.h @@ -40,6 +41,7 @@ libbaton_la_SOURCES = baton.c \ operations.c \ query.c \ read.c \ + signal_handler.c \ utilities.c \ write.c diff --git a/src/baton.c b/src/baton.c index e6cb2cd2..ac271721 100644 --- a/src/baton.c +++ b/src/baton.c @@ -32,6 +32,7 @@ #include "config.h" #include "baton.h" +#include "signal_handler.h" static const char *metadata_op_name(metadata_op op) { const char *name; @@ -69,23 +70,14 @@ static rcComm_t *rods_connect(rodsEnv *env){ rcComm_t *conn = NULL; rErrMsg_t errmsg; - // Override the signal handler installed by the iRODS client - // library (all versions up to 4.1.7, inclusive) because it - // detects the signal and leaves the program running in a tight - // loop. Here we ignore SIGPIPE and fail on read/write. - struct sigaction saction; - saction.sa_handler = SIG_IGN; - saction.sa_flags = 0; - sigemptyset(&saction.sa_mask); - // TODO: add option for NO_RECONN vs. RECONN_TIMEOUT conn = rcConnect(env->rodsHost, env->rodsPort, env->rodsUserName, env->rodsZone, NO_RECONN, &errmsg); + if (!conn) goto finally; - int sigstatus = sigaction(SIGPIPE, &saction, NULL); + int sigstatus = apply_signal_handler(); if (sigstatus != 0) { - logmsg(FATAL, "Failed to set the iRODS client SIGPIPE handler"); exit(1); } diff --git a/src/list.c b/src/list.c index 6f100995..1e751fbe 100644 --- a/src/list.c +++ b/src/list.c @@ -212,6 +212,7 @@ json_t *list_checksum(rcComm_t *conn, rodsPath_t *rods_path, query_in = make_query_input(SEARCH_MAX_ROWS, obj_format.num_columns, obj_format.columns); query_in = prepare_obj_list(query_in, rods_path, NULL); + query_in = limit_to_good_repl(query_in); results = do_query(conn, query_in, obj_format.labels, error); if (error->code != 0) goto error; diff --git a/src/operations.c b/src/operations.c index 3efff3ec..96fa4d88 100644 --- a/src/operations.c +++ b/src/operations.c @@ -34,7 +34,7 @@ static int iterate_json(FILE *input, rodsEnv *env, baton_json_op fn, int drop_conn_count = 0; int status = 0; - while (!feof(input)) { + while (!exit_flag && !feof(input)) { size_t jflags = JSON_DISABLE_EOF_CHECK | JSON_REJECT_DUPLICATES; json_error_t load_error; json_t *item = json_loadf(input, jflags, &load_error); // JSON alloc @@ -154,6 +154,12 @@ static int iterate_json(FILE *input, rodsEnv *env, baton_json_op fn, } } // while + if (exit_flag) { + status = exit_flag; + logmsg(WARN, "Exiting on signal with code %d", exit_flag); + goto finally; + } + if (drop_conn_count > 0) { logmsg(WARN, "Reconnected for put operations %d times", drop_conn_count); @@ -168,30 +174,35 @@ static int iterate_json(FILE *input, rodsEnv *env, baton_json_op fn, int do_operation(FILE *input, baton_json_op fn, operation_args_t *args) { int item_count = 0; int error_count = 0; - + int status = 0; + rodsEnv env; - if (!input) goto error; + if (!input) { + status = 1; + goto error; + } - int status = iterate_json(input, &env, fn, args, &item_count, &error_count); + status = iterate_json(input, &env, fn, args, &item_count, &error_count); if (status != 0) goto error; if (error_count > 0) { logmsg(WARN, "Processed %d items with %d errors", item_count, error_count); + status = 1; } else { logmsg(DEBUG, "Processed %d items with %d errors", item_count, error_count); } - return error_count; + return status; error: logmsg(ERROR, "Processed %d items with %d errors", item_count, error_count); - return 1; + return status; } json_t *baton_json_dispatch_op(rodsEnv *env, rcComm_t *conn, json_t *envelope, diff --git a/src/operations.h b/src/operations.h index 3f31140d..9aa501f8 100644 --- a/src/operations.h +++ b/src/operations.h @@ -27,6 +27,7 @@ #include #include "config.h" +#include "signal_handler.h" /** * @enum metadata_op @@ -131,7 +132,8 @@ typedef json_t *(*baton_json_op) (rodsEnv *env, * @param[in] zone_name Zone name, char * (optional). * @param[in] buffer_size Data transfer buffer, size_t (optional). * - * @return 0 on success, iRODS error code on failure. + * @return 0 on success, error code on failure. The error code is suitable + * for use as an exit code for the program calling do_operation. */ int do_operation(FILE *input, baton_json_op fn, operation_args_t *args); diff --git a/src/read.c b/src/read.c index d5f352ce..325aa676 100644 --- a/src/read.c +++ b/src/read.c @@ -233,6 +233,7 @@ size_t read_chunk(rcComm_t *conn, data_obj_file_t *data_obj, char *buffer, set_baton_error(error, num_read, "Failed to read up to %zu bytes from '%s': %s", len, data_obj->path, err_name); + num_read = 0; goto finally; } diff --git a/src/signal_handler.c b/src/signal_handler.c new file mode 100644 index 00000000..7d2959cf --- /dev/null +++ b/src/signal_handler.c @@ -0,0 +1,73 @@ +/** + * Copyright (C) 2021 Genome + * Research Ltd. All rights reserved. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + * + * @file signal_handler.c + * @author Michael Kubiak + */ + +#include "signal_handler.h" +#include "baton.h" + +#include + +int signals[] = {SIGINT, SIGQUIT, SIGHUP, SIGTERM, SIGUSR1, SIGUSR2, SIGPIPE, 0}; +int exit_flag; + +void handle_signal(int signal){ + switch (signal) { + case SIGINT: + exit_flag = 2; + break; + case SIGQUIT: + exit_flag = 3; + break; + case SIGHUP: + exit_flag = 4; + break; + case SIGTERM: + case SIGUSR1: + case SIGUSR2: + exit_flag = 5; + break; + case SIGPIPE: + exit_flag = 6; + break; + default: + exit_flag = 1; + } +} + +int apply_signal_handler() { + exit_flag = 0; + + struct sigaction saction; + saction.sa_handler = &handle_signal; + saction.sa_flags = 0; + sigemptyset(&saction.sa_mask); + int sigstatus; + + // Exit gracefully on fatal signals + for (int i = 0; signals[i] != 0; i++) { + sigstatus = sigaction(signals[i], &saction, NULL); + if (sigstatus != 0) { + logmsg(FATAL, "Failed to set the iRODS client handler for signal %s", strsignal(signals[i])); + return -1; + } + } + + return 0; +} diff --git a/src/signal_handler.h b/src/signal_handler.h new file mode 100644 index 00000000..f8ce7c29 --- /dev/null +++ b/src/signal_handler.h @@ -0,0 +1,24 @@ +/** + * Copyright (C) 2021 Genome + * Research Ltd. All rights reserved. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + * + * @file signal_handler.h + * @author Michael Kubiak + */ + +extern int exit_flag; +void handle_signal(int signal); +int apply_signal_handler(); diff --git a/tests/check_baton.c b/tests/check_baton.c index b3e86b5c..69f22cb5 100644 --- a/tests/check_baton.c +++ b/tests/check_baton.c @@ -30,13 +30,16 @@ #include "../src/log.h" #include "../src/read.h" #include "../src/compat_checksum.h" +#include "../src/signal_handler.h" + +int exit_flag; static int MAX_COMMAND_LEN = 1024; static int MAX_PATH_LEN = 4096; -static char *TEST_COLL = "baton-basic-test"; -static char *TEST_DATA_PATH = "data"; -static char *TEST_METADATA_PATH = "metadata/meta1.imeta"; +static char *TEST_COLL = "baton-basic-test"; +static char *TEST_DATA_PATH = "data"; +static char *TEST_METADATA_PATH = "metadata/meta1.imeta"; static char *SQL_PATH = "sql/specific_queries.sql"; static char *SETUP_SCRIPT = "scripts/setup_irods.sh"; @@ -45,6 +48,8 @@ static char *SQL_SETUP_SCRIPT = "scripts/setup_sql.sh"; static char *TEARDOWN_SCRIPT = "scripts/teardown_irods.sh"; static char *SQL_TEARDOWN_SCRIPT = "scripts/teardown_sql.sh"; +static char *BAD_REPLICA_SCRIPT = "scripts/make_bad_replica.sh"; + static void set_current_rods_root(char *in, char *out) { rodsEnv rodsEnv; @@ -112,12 +117,16 @@ static void basic_teardown() { char rods_root[MAX_PATH_LEN]; set_current_rods_root(TEST_COLL, rods_root); - snprintf(command, MAX_COMMAND_LEN, "%s/%s %s", + rodsEnv env; + int ret = getRodsEnv(&env); + if (ret != 0) raise(SIGTERM); + + snprintf(command, MAX_COMMAND_LEN, "%s/%s %s %s", TEST_ROOT, TEARDOWN_SCRIPT, - rods_root); + env.rodsUserName, rods_root); printf("Data teardown: %s\n", command); - int ret = system(command); + ret = system(command); if (ret != 0) raise(SIGINT); } @@ -2226,6 +2235,56 @@ START_TEST(test_checksum_data_obj) { } END_TEST +// Can we checksum an object, ignoring stale replicas +START_TEST(test_checksum_ignore_stale) { + option_flags flags = 0; + rodsEnv env; + rcComm_t *conn = rods_login(&env); + + char rods_root[MAX_PATH_LEN]; + set_current_rods_root(TEST_COLL, rods_root); + + char obj_name[38] = "lorem_10k.txt"; + char obj_path[MAX_PATH_LEN]; + snprintf(obj_path, MAX_PATH_LEN, "%s/%s", + rods_root, obj_name); + + rodsPath_t rods_obj_path; + baton_error_t resolve_error; + resolve_rods_path(conn, &env, &rods_obj_path, obj_path, + flags, &resolve_error); + ck_assert_int_eq(resolve_error.code, 0); + + char command[MAX_COMMAND_LEN]; + // change checksum of replica 1 without marking stale + snprintf(command, MAX_COMMAND_LEN, "%s/%s -c %s -d %s -a", TEST_ROOT, + BAD_REPLICA_SCRIPT, rods_root, obj_name); + + int ret = system(command); + if (ret != 0) raise(SIGTERM); + + baton_error_t wrong_checksum_error; + json_t *result = list_path(conn, &rods_obj_path, PRINT_CHECKSUM, + &wrong_checksum_error); + ck_assert_int_ne(wrong_checksum_error.code, 0); + + // mark replica 1 as stale + snprintf(command, MAX_COMMAND_LEN, "%s/%s -c %s -d %s -s", TEST_ROOT, + BAD_REPLICA_SCRIPT, rods_root, obj_name); + + ret = system(command); + if (ret != 0) raise(SIGTERM); + + baton_error_t stale_replica_error; + result = list_path(conn, &rods_obj_path, PRINT_CHECKSUM, + &stale_replica_error); + ck_assert_int_eq(stale_replica_error.code, 0); + + json_decref(result); + +} +END_TEST + // Can we remove a data object START_TEST(test_remove_data_obj) { option_flags flags = 0; @@ -2529,6 +2588,20 @@ START_TEST(test_search_specific_with_valid_setup) { } END_TEST +START_TEST(test_exit_flag_on_sigint) { + apply_signal_handler(); + raise(SIGINT); + ck_assert(exit_flag == 2); +} +END_TEST + +START_TEST(test_exit_flag_on_sigterm) { + apply_signal_handler(); + raise(SIGTERM); + ck_assert(exit_flag == 5); +} +END_TEST + // Having metadata on an item of (a = x, a = y), a search for "a = x" // gives correct results, as does a search for "a = y". However, // searching for "a = x and a = y" does not (nothing is returned). @@ -2741,6 +2814,56 @@ START_TEST(test_regression_github_issue242) { ck_assert(json_equal(json_object_get(result, JSON_CHECKSUM_KEY), json_string("d41d8cd98f00b204e9800998ecf8427e"))); } +END_TEST + +START_TEST(test_regression_github_issue252) { + option_flags flags = 0; + rodsEnv env; + rcComm_t *conn = rods_login(&env); + + char rods_root[MAX_PATH_LEN]; + set_current_rods_root(TEST_COLL, rods_root); + char obj_path[MAX_PATH_LEN]; + snprintf(obj_path, MAX_PATH_LEN, "%s/lorem_1k.txt", rods_root); + + rodsPath_t rods_path; + baton_error_t resolve_error; + ck_assert_int_eq(resolve_rods_path(conn, &env, &rods_path, obj_path, + flags, &resolve_error), EXIST_ST); + + // Remove read access to trigger the bug + char ichmod_r[MAX_COMMAND_LEN]; + snprintf(ichmod_r, MAX_COMMAND_LEN, "ichmod null %s %s", + env.rodsUserName, obj_path); + + int ret = system(ichmod_r); + ck_assert_msg(ret == 0, ichmod_r); + + baton_error_t sver_error; + char *server_version = get_server_version(conn, &sver_error); + ck_assert_int_eq(sver_error.code, 0); + + baton_error_t open_error; + data_obj_file_t *obj = open_data_obj(conn, &rods_path, + O_RDONLY, 0, &open_error); + + // iRODS 4.2.7 lets you "open" a data object you can't read, but you + // can't get bytes from it, giving the following error + if (str_equals(server_version, "4.2.7", MAX_STR_LEN)) { + baton_error_t slurp_error; + slurp_data_obj(conn, obj, 512, &slurp_error); + + // Furthermore, iRODS 4.2.7 gives the inappropriate error + // -190000 SYS_FILE_DESC_OUT_OF_RANGE if it gets this far. + ck_assert_int_eq(slurp_error.code, -19000); + ck_assert_int_eq(close_data_obj(conn, obj), -19000); + } else { + ck_assert_int_eq(open_error.code, -818000); + } + + if (conn) rcDisconnect(conn); +} +END_TEST Suite *baton_suite(void) { Suite *suite = suite_create("baton"); @@ -2767,7 +2890,7 @@ Suite *baton_suite(void) { tcase_add_test(basic, test_init_rods_path); tcase_add_test(basic, test_resolve_rods_path); tcase_add_test(basic, test_make_query_input); - + TCase *path = tcase_create("path"); tcase_add_unchecked_fixture(path, setup, teardown); tcase_add_checked_fixture(path, basic_setup, basic_teardown); @@ -2814,6 +2937,7 @@ Suite *baton_suite(void) { tcase_add_test(read_write, test_write_data_obj); tcase_add_test(read_write, test_put_data_obj); tcase_add_test(read_write, test_checksum_data_obj); + tcase_add_test(read_write, test_checksum_ignore_stale); tcase_add_test(read_write, test_remove_data_obj); tcase_add_test(read_write, test_create_coll); tcase_add_test(read_write, test_remove_coll); @@ -2847,6 +2971,12 @@ Suite *baton_suite(void) { tcase_add_test(specific_query, test_search_specific_with_valid_setup); + TCase *signal_handler = tcase_create("signal_handler"); + tcase_add_unchecked_fixture(signal_handler, setup, teardown); + tcase_add_checked_fixture(signal_handler, basic_setup, basic_teardown); + tcase_add_test(signal_handler, test_exit_flag_on_sigint); + tcase_add_test(signal_handler, test_exit_flag_on_sigterm); + TCase *regression = tcase_create("regression"); tcase_add_unchecked_fixture(regression, setup, teardown); tcase_add_checked_fixture(regression, basic_setup, basic_teardown); @@ -2855,6 +2985,7 @@ Suite *baton_suite(void) { tcase_add_test(regression, test_regression_github_issue137); tcase_add_test(regression, test_regression_github_issue140); tcase_add_test(regression, test_regression_github_issue242); + tcase_add_test(regression, test_regression_github_issue252); suite_add_tcase(suite, utilities); suite_add_tcase(suite, basic); @@ -2863,6 +2994,7 @@ Suite *baton_suite(void) { suite_add_tcase(suite, read_write); suite_add_tcase(suite, json); suite_add_tcase(suite, specific_query); + suite_add_tcase(suite, signal_handler); suite_add_tcase(suite, regression); return suite; diff --git a/tests/scripts/make_bad_replica.sh b/tests/scripts/make_bad_replica.sh new file mode 100755 index 00000000..b078b830 --- /dev/null +++ b/tests/scripts/make_bad_replica.sh @@ -0,0 +1,128 @@ +#!/usr/bin/env bash + +set -eo pipefail + +usage() { + cat 1>&2 <] + [-d ] + [-r ] + [-i ] + [-s] [-a] [-o] + [-v] + [-h] + +Options + -h Display usage and exit + -v verbose + -c path to collection + -d data object in name + -r replica number to alter + -i irods server container name + -s mark as stale + -a alter checksum + -o corrupt object contents + +EOF +} + +log () { + if [[ $verbose == 1 ]] ; then + printf "${1}" + fi +} + +ils_error() { + echo "Error running ils, object may not exist or iinit may not have been run" + exit 1 +} + +#Default values +repl_num=1 +irods_container="irods-server" + +while getopts "hvc:d:r:i:sao" option; do + case "$option" in + h) + usage + exit 0 + ;; + v) + verbose=1 + ;; + c) + coll="$OPTARG" + ;; + d) + obj="$OPTARG" + ;; + r) + repl_num="$OPTARG" + ;; + i) + irods_container="$OPTARG" + ;; + s) + stale=1 + ;; + a) + check=1 + ;; + o) + corrupt=1 + ;; + + esac +done + +if [[ -z $coll ]] ; then + printf "Collection path required" + usage + exit 1 +elif [[ -z $obj ]] ; then + printf "Object name required" + usage + exit 1 +else + exists=$(ils "$coll/$obj") || ils_error +fi + +log "Object: $coll/$obj \nReplica: $repl_num \nOperations: \n" +updates="" + +if [[ -n $stale ]] ; then + log " - make stale \n" + updates+="DATA_IS_DIRTY='0' " + op=1 +fi + +if [[ -n $check ]] ; then + log " - alter checksum \n" + updates+="DATA_CHECKSUM='0' " + op=1 +fi + +if [[ -n $corrupt ]] ; then + log " - corrupt data \n" + fs_path=$(ils -L $coll/$obj | grep /var/lib/irods | sed "$(($repl_num+1))q;d" | sed -e 's/.*generic //g') + docker exec -u root "$irods_container" bash -c "echo 'This file is corrupted' > $fs_path" || exit 1; + op=2 +fi + +if [[ -z $op ]]; then + printf "No operation requested \n" + exit 0 +fi + + +if [[ $op -eq 1 ]]; then + docker exec -u root "$irods_container" sudo -u irods psql -d ICAT -c "UPDATE r_data_main d SET $updates FROM r_coll_main c WHERE d.coll_id = c.coll_id AND d.DATA_NAME='$obj' AND c.COLL_NAME='$coll' AND d.DATA_REPL_NUM='$repl_num';" || exit 1 +fi + + diff --git a/tests/scripts/teardown_irods.sh b/tests/scripts/teardown_irods.sh index 1f9bc66d..6885a78c 100755 --- a/tests/scripts/teardown_irods.sh +++ b/tests/scripts/teardown_irods.sh @@ -6,14 +6,16 @@ E_ARGS_MISSING=3 -in_path=$1 +owner="$1" +in_path="$2" -if [[ $# < 1 ]] +if [[ $# < 2 ]] then - echo "Insufficient command line arguments; expected 1" + echo "Insufficient command line arguments; expected 2" exit $E_ARGS_MISSING fi -irm -rf $in_path +ichmod -r own "$owner" "$in_path" +irm -rf "$in_path" exit