On Wed, Jan 24, 2018 at 1:42 AM, Richard Haines
<richard_c_haines@xxxxxxxxxxxxxx> wrote:
> Allow the tmp build files to be kept for debugging when a policy
> build fails.
>
> Signed-off-by: Richard Haines <richard_c_haines@xxxxxxxxxxxxxx>
> ---
> V2 Changes:
> Remove the retain-tmp flag and just keep tmp files on build errors.
> V3 Changes:
> Release transaction lock after tmp files removed.
> Add additional comments to commit_err in handle.h
>
> libsemanage/src/direct_api.c | 56 ++++++++++++++++++++++++++++++--------------
> libsemanage/src/handle.c | 2 ++
> libsemanage/src/handle.h | 4 ++++
> 3 files changed, 44 insertions(+), 18 deletions(-)
>
> diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
> index a455612f..88873c43 100644
> --- a/libsemanage/src/direct_api.c
> +++ b/libsemanage/src/direct_api.c
> @@ -323,25 +323,43 @@ static void semanage_direct_destroy(semanage_handle_t * sh
> /* do nothing */
> }
>
> -static int semanage_direct_disconnect(semanage_handle_t * sh)
> +static int semanage_remove_tmps(semanage_handle_t *sh)
> {
> - /* destroy transaction */
> - if (sh->is_in_transaction) {
> - /* destroy sandbox */
> - if (semanage_remove_directory
> - (semanage_path(SEMANAGE_TMP, SEMANAGE_TOPLEVEL)) < 0) {
> + if (sh->commit_err)
> + return 0;
> +
> + /* destroy sandbox if it exists */
> + if (semanage_remove_directory
> + (semanage_path(SEMANAGE_TMP, SEMANAGE_TOPLEVEL)) < 0) {
> + if (errno != ENOENT) {
> ERR(sh, "Could not cleanly remove sandbox %s.",
> semanage_path(SEMANAGE_TMP, SEMANAGE_TOPLEVEL));
> return -1;
> }
> - if (semanage_remove_directory
> - (semanage_final_path(SEMANAGE_FINAL_TMP,
> - SEMANAGE_FINAL_TOPLEVEL)) < 0) {
> + }
> +
> + /* destroy tmp policy if it exists */
> + if (semanage_remove_directory
> + (semanage_final_path(SEMANAGE_FINAL_TMP,
> + SEMANAGE_FINAL_TOPLEVEL)) < 0) {
> + if (errno != ENOENT) {
> ERR(sh, "Could not cleanly remove tmp %s.",
> semanage_final_path(SEMANAGE_FINAL_TMP,
> SEMANAGE_FINAL_TOPLEVEL));
> return -1;
> }
> + }
> +
> + return 0;
> +}
> +
> +static int semanage_direct_disconnect(semanage_handle_t *sh)
> +{
> + int retval = 0;
> +
> + /* destroy transaction and remove tmp files if no commit error */
> + if (sh->is_in_transaction) {
> + retval = semanage_remove_tmps(sh);
> semanage_release_trans_lock(sh);
> }
>
> @@ -375,7 +393,7 @@ static int semanage_direct_disconnect(semanage_handle_t * sh)
> /* Release object databases: active kernel policy */
> bool_activedb_dbase_release(semanage_bool_dbase_active(sh));
>
> - return 0;
> + return retval;
> }
>
> static int semanage_direct_begintrans(semanage_handle_t * sh)
> @@ -1635,17 +1653,19 @@ cleanup:
> free(mod_filenames);
> sepol_policydb_free(out);
> cil_db_destroy(&cildb);
> - semanage_release_trans_lock(sh);
>
> free(fc_buffer);
>
> - /* regardless if the commit was successful or not, remove the
> - sandbox if it is still there */
> - semanage_remove_directory(semanage_path
> - (SEMANAGE_TMP, SEMANAGE_TOPLEVEL));
> - semanage_remove_directory(semanage_final_path
> - (SEMANAGE_FINAL_TMP,
> - SEMANAGE_FINAL_TOPLEVEL));
> + /* Set commit_err so other functions can detect any errors. Note that
> + * retval > 0 will be the commit number.
> + */
> + if (retval < 0)
> + sh->commit_err = retval;
> +
> + if (semanage_remove_tmps(sh) != 0)
> + retval = -1;
> +
> + semanage_release_trans_lock(sh);
> umask(mask);
>
> return retval;
> diff --git a/libsemanage/src/handle.c b/libsemanage/src/handle.c
> index 4ce1df03..a6567bd4 100644
> --- a/libsemanage/src/handle.c
> +++ b/libsemanage/src/handle.c
> @@ -86,6 +86,8 @@ semanage_handle_t *semanage_handle_create(void)
> * If any changes are made, this flag is ignored */
> sh->do_rebuild = 0;
>
> + sh->commit_err = 0;
> +
> /* By default always reload policy after commit if SELinux is enabled. */
> sh->do_reload = (is_selinux_enabled() > 0);
>
> diff --git a/libsemanage/src/handle.h b/libsemanage/src/handle.h
> index 1780ac81..a91907b0 100644
> --- a/libsemanage/src/handle.h
> +++ b/libsemanage/src/handle.h
> @@ -62,6 +62,10 @@ struct semanage_handle {
> int is_in_transaction;
> int do_reload; /* whether to reload policy after commit */
> int do_rebuild; /* whether to rebuild policy if there were no changes */
> + int commit_err; /* set by semanage_direct_commit() if there are
> + * any errors when building or committing the
> + * sandbox to kernel policy at /etc/selinux
> + */
> int modules_modified;
> int create_store; /* whether to create the store if it does not exist
> * this will only have an effect on direct connections */
> --
> 2.14.3
>
>
LGTM. Stephen any other comments? I have this going through the CI
tests as we speak.
Bill
