From patchwork Mon Apr 1 05:53:32 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: 'Darko Komljenovic' via swupdate X-Patchwork-Id: 1072350 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=googlegroups.com (client-ip=2607:f8b0:4864:20::d3c; helo=mail-io1-xd3c.google.com; envelope-from=swupdate+bncbcg4lpfp6aorb2woq3sqkgqe6isltea@googlegroups.com; receiver=) Authentication-Results: ozlabs.org; dmarc=pass (p=none dis=none) header.from=googlegroups.com Authentication-Results: ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=googlegroups.com header.i=@googlegroups.com header.b="f1a+5B9w"; dkim-atps=neutral Received: from mail-io1-xd3c.google.com (mail-io1-xd3c.google.com [IPv6:2607:f8b0:4864:20::d3c]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 44XhNb6cTCz9s7h for ; Mon, 1 Apr 2019 16:53:49 +1100 (AEDT) Received: by mail-io1-xd3c.google.com with SMTP id c2sf7257641ioh.11 for ; Sun, 31 Mar 2019 22:53:49 -0700 (PDT) ARC-Seal: i=2; a=rsa-sha256; t=1554098026; cv=pass; d=google.com; s=arc-20160816; b=ggI7Vt25S+Irzv5pQQjRzdkLPgMVPIeFVw4DbvH1izaXoDT8Wa3LqQW4LAG7gwaNrB t/z1QczMGHGt6IwR2uEEprcpYT984+wz4j8au7JT/fldS4hXo7xorbCkUEA5bFDoe36+ jC9TYnD0o+r6nYuGzByPoH2EG1uT1SmpJwIDVAQfBhBo+7yBvcV9xiSmZAHnfYtsIMUe je48ltfdQyKT6jqG1PtBuc0MnOSzZvrntx9eERHxhEYE8L5Bz3hcZwpysXN8YDxLGLl4 V3/OxTlF1TrD0z0UPH2PwSsc1d1YQyhUJ24w24uKvbjhJtonBoddOmfD2xNLDfaxc1PT 8K4g== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-unsubscribe:list-subscribe:list-archive:list-help:list-post :list-id:mailing-list:precedence:reply-to:message-id:date:subject:cc :to:from:mime-version:dkim-signature; bh=ssFcu3TCvEk/tRzkpIUJ00uxoY9tcNjoLkhk0eIAZfU=; b=dGldlAKtcEA00ErHQ3gbyFtU3Xi9i9ep7/jz/wdNGvzYnMwMfyVM9t3A/p8ydxxWIe hyvWrCZ+2idWjgO4JilbX7YV0yHwdUvzkPLx6RhlRpuPnrPhFTS6XdLye0FNiQG4svcu AMJP/+7DfFHQPmHysNNfW/wZfSlvuRutlkookian9rHTw/ikxLNu4GAmwGcOUhef9SGW 9uVrv6Y26JV/1nHAkikJ84Yid/wJJ7TV0dXDFDYU0ci8GKHlsLFOzIAug2I1Eocxweot Xr0zAtvs5Gjy0nwBx05EdO4sGesQDVHqMh9s3Ki0D6aNPUxhfsYzIv3tmW/FcOMum3XX lkzw== ARC-Authentication-Results: i=2; gmr-mx.google.com; dkim=pass header.i=@planetinnovation.com.au header.s=google header.b=CmC4MupB; spf=pass (google.com: domain of austin.phillips@planetinnovation.com.au designates 2607:f8b0:4864:20::543 as permitted sender) smtp.mailfrom=austin.phillips@planetinnovation.com.au; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=planetinnovation.com.au DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=googlegroups.com; s=20161025; h=mime-version:from:to:cc:subject:date:message-id:x-original-sender :x-original-authentication-results:reply-to:precedence:mailing-list :list-id:list-post:list-help:list-archive:list-subscribe :list-unsubscribe; bh=ssFcu3TCvEk/tRzkpIUJ00uxoY9tcNjoLkhk0eIAZfU=; b=f1a+5B9wzNh1JoAfmRU80ZBnctoOU/OHj1jcf+3Xqrbjjm2rK8GFuMMGxmrfLVA4fb da/aC5KNQ2n/zvLtQKe1QLroNtmYbYIOi5NMMAANQY0FVN7BoTCUKDkZ0QDOyHbZvCft /JLSCsXgxFYsQB+U9f6Hd/uhjeGjyw+Oxk5CkAI64McMFBtlkvJ3Vn85nTSp1zfM1l6s g8ZUhW6rUY5SiBlNma7fOuAJfzkOb7NEKdxy02ZPJ3LCwaITJ4zkGpiLPrQ088isLUrC yrK/+1rF3BBAK30Z24cKdQaqnpVtwFS9MYNqLPC2CzyJp62SGQKMTc1ItGLB4DZ3yLXO q6qA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:from:to:cc:subject:date:message-id :x-original-sender:x-original-authentication-results:reply-to :precedence:mailing-list:list-id:x-spam-checked-in-group:list-post :list-help:list-archive:list-subscribe:list-unsubscribe; bh=ssFcu3TCvEk/tRzkpIUJ00uxoY9tcNjoLkhk0eIAZfU=; b=tCGqXV1+TRYT58Y4iDrXjHkXlryNB4SJywBS1ZNQ1iiYVgDtT1mYScMpPA+YuQ3CW+ v8oqF9mm76OdrwRxl+RH8rgJbcAX/mdDQmIyF1u+/vSlIqATremZrYdvsfjXq+KfgJvM +S+dvMNIsOKSAwgtu5ocjx6g32IOg9lzH46bq+rbmj9rorHYrSJxYX6ECpHdEsXVl+1s kQXXY3OVVroQH6nPWbKq5wA89zLHQaSRqYRuy1rVI30gdar4U2YW5JQAL7PTYlkniMwV wAoO//4i6P92me6DTss0hxnL2BMJvkXFWBLVRVb5hB65R73SIjTNVMqTpfvxQOk1Yj+N dcVg== X-Gm-Message-State: APjAAAWdCXRGUessXfvpiYjtfQ3XlKrdA1IF/mfjducozFGiTr3iXCMG n0DO+kYrp4mqHOYdplRESJs= X-Google-Smtp-Source: APXvYqyOQszUQPpk/HrGISBLhar67mUZwjby3mCHZBQ0nC0ZfeDhf2eRquXJ3ZpeOACIhjmRN4tfiA== X-Received: by 2002:a24:3c4c:: with SMTP id m73mr14689762ita.23.1554098026604; Sun, 31 Mar 2019 22:53:46 -0700 (PDT) X-BeenThere: swupdate@googlegroups.com Received: by 2002:a24:a01:: with SMTP id 1ls2430757itw.3.gmail; Sun, 31 Mar 2019 22:53:46 -0700 (PDT) X-Received: by 2002:a24:73cb:: with SMTP id y194mr6076364itb.6.1554098026144; Sun, 31 Mar 2019 22:53:46 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1554098026; cv=none; d=google.com; s=arc-20160816; b=Nv15lDn3VdwWwgJdr0eXAJ9bpYjLmnwD2Ml7fLHPq7ZIpgSOiMAXiYq4h/ltrU+AVh TMwNh/gzNGVogt1rNmvkmZlIvW5wTUZsvQztt6n+CzSXDJobhpkw0NFHg20RdK1DCElM 5xP4aUWS4s8sriaX82gir3mgFdxR65P7qHsMiA28AkgmDAaWIRTsYqQlVMG2kYv77cmT Zl+5RCBnPHL0A3+Jda2l84JTKgr0gvsMEsD4XtW6IvGF8QMcuU5l/EN9sevvcUS7YQxd eHi9dCjAIKdVVv5aK2pYy0dlHbIjASpy5Sp4lnIoUYDw6ocvVcKAQhKXdh//A/d3541m lcaQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=message-id:date:subject:cc:to:from:mime-version:dkim-signature; bh=5sSe9s/PvR5WNt5NfUoYis1cSityZLyCrXQDWFwoG+A=; b=Gbb7HTEMIYSG4ijNUfm5oJnsyYRolx13Dz0nxChtW+i05hNGPNHFQxE15aEemBHPky yiMqN0axRoORnpzEC95M0I8NGDM4+z9AybL3n3jnrRIidno8JEY+M3zYL7HBwC2J1FDc pkB///YrzcgzeyjjI72TCkrf8p0ZRhBkVtNsK8C8u++VLqFBAhpqIAuFC7T3C8ifa/py 6Yj6NZ2TyFrtStWlcAOujB6zYAAo0TbDBCYQeoKJCH0jb1KDJPlUBa+IHLGYuDro7SiL ACDj8wNU8Iyu+6feT7V2vAEK1OZvo2gltKJz2ZpVfpEP18ruckuYB8nFT2M+bCCJwiZn beEg== ARC-Authentication-Results: i=1; gmr-mx.google.com; dkim=pass header.i=@planetinnovation.com.au header.s=google header.b=CmC4MupB; spf=pass (google.com: domain of austin.phillips@planetinnovation.com.au designates 2607:f8b0:4864:20::543 as permitted sender) smtp.mailfrom=austin.phillips@planetinnovation.com.au; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=planetinnovation.com.au Received: from mail-pg1-x543.google.com (mail-pg1-x543.google.com. [2607:f8b0:4864:20::543]) by gmr-mx.google.com with ESMTPS id w136si459415itb.2.2019.03.31.22.53.46 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sun, 31 Mar 2019 22:53:46 -0700 (PDT) Received-SPF: pass (google.com: domain of austin.phillips@planetinnovation.com.au designates 2607:f8b0:4864:20::543 as permitted sender) client-ip=2607:f8b0:4864:20::543; Received: by mail-pg1-x543.google.com with SMTP id y3so4155600pgk.12 for ; Sun, 31 Mar 2019 22:53:46 -0700 (PDT) MIME-Version: 1.0 X-Received: by 2002:a63:78a:: with SMTP id 132mr51917438pgh.196.1554098025235; Sun, 31 Mar 2019 22:53:45 -0700 (PDT) Received: from localhost.localdomain (gen-180-189-155-42.ptr4.otw.net.au. [180.189.155.42]) by smtp.googlemail.com with ESMTPSA id k72sm18991625pfb.122.2019.03.31.22.53.43 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Sun, 31 Mar 2019 22:53:44 -0700 (PDT) X-Patchwork-Original-From: "'Austin Phillips' via swupdate" From: 'Darko Komljenovic' via swupdate To: swupdate@googlegroups.com Cc: Austin Phillips Subject: [swupdate] [PATCH] handlers: Improve archive_handler error handling Date: Mon, 1 Apr 2019 16:53:32 +1100 Message-Id: <1554098012-105033-1-git-send-email-austin.phillips@planetinnovation.com.au> X-Mailer: git-send-email 2.7.4 X-Original-Sender: austin.phillips@planetinnovation.com.au X-Original-Authentication-Results: gmr-mx.google.com; dkim=pass header.i=@planetinnovation.com.au header.s=google header.b=CmC4MupB; spf=pass (google.com: domain of austin.phillips@planetinnovation.com.au designates 2607:f8b0:4864:20::543 as permitted sender) smtp.mailfrom=austin.phillips@planetinnovation.com.au; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=planetinnovation.com.au X-Original-From: Austin Phillips Reply-To: Austin Phillips Precedence: list Mailing-list: list swupdate@googlegroups.com; contact swupdate+owners@googlegroups.com List-ID: X-Spam-Checked-In-Group: swupdate@googlegroups.com X-Google-Group-Id: 605343134186 List-Post: , List-Help: , List-Archive: , List-Unsubscribe: , This change makes a number of changes to the archive handler to ensure that correct cleanup of resources occurs both during normal operation and in error scenarios. Remove memory leaks by freeing memory allocated during archive copy. Modify control flow so that exit from copy and extract functions always go through an unwind stage to free any memory and resource allocations and close all opened handles. Adjust archive FIFO creation permissions from 0666 to 0660 to improve system security during archive extraction and prevent modification of extraction process by other parts of the system that don't have FIFO file access. Signed-off-by: Austin Phillips --- handlers/archive_handler.c | 142 ++++++++++++++++++++++++++++++++------------- 1 file changed, 101 insertions(+), 41 deletions(-) diff --git a/handlers/archive_handler.c b/handlers/archive_handler.c index a44819d..cbac0fa 100644 --- a/handlers/archive_handler.c +++ b/handlers/archive_handler.c @@ -67,16 +67,25 @@ copy_data(struct archive *ar, struct archive *aw) static void * extract(void *p) { - struct archive *a; - struct archive *ext; - struct archive_entry *entry; + struct archive *a = NULL; + struct archive *ext = NULL; + struct archive_entry *entry = NULL; int r; int flags; struct extract_data *data = (struct extract_data *)p; flags = data->flags; + int exitval = -EFAULT; a = archive_read_new(); + if (a == NULL) { + goto out; + } + ext = archive_write_disk_new(); + if (ext == NULL) { + goto out; + } + archive_write_disk_set_options(ext, flags); /* * Note: archive_write_disk_set_standard_lookup() is useful @@ -97,7 +106,7 @@ extract(void *p) if ((r = archive_read_open_filename(a, FIFO, 4096))) { ERROR("archive_read_open_filename(): %s %d", archive_error_string(a), r); - pthread_exit((void *)-1); + goto out; } for (;;) { r = archive_read_next_header(a, &entry); @@ -106,7 +115,7 @@ extract(void *p) if (r != ARCHIVE_OK) { ERROR("archive_read_next_header(): %s %d", archive_error_string(a), 1); - pthread_exit((void *)-1); + goto out; } if (debug) @@ -122,28 +131,47 @@ extract(void *p) if (r != ARCHIVE_OK) { ERROR("archive_write_finish_entry(): %s", archive_error_string(ext)); - pthread_exit((void *)-1); + goto out; } } } - archive_read_close(a); - archive_read_free(a); - pthread_exit((void *)0); + exitval = 0; + +out: + if (ext != NULL) { + r = archive_write_free(ext); + if (r) { + ERROR("archive_write_free(): %s %d", + archive_error_string(a), r); + exitval = -EFAULT; + } + } + + if (a != NULL) { + archive_read_close(a); + archive_read_free(a); + } + + pthread_exit((void *)exitval); } static int install_archive_image(struct img_type *img, void __attribute__ ((__unused__)) *data) { char path[255]; - int fdout; - int ret = 0; - char pwd[256]; + int fdout = -1; + int ret = -1; + int thread_ret = -1; + char pwd[256] = "\0"; struct extract_data tf; pthread_attr_t attr; void *status; int use_mount = (strlen(img->device) && strlen(img->filesystem)) ? 1 : 0; + int is_mounted = 0; + int exitval = -EFAULT; + char* FIFO = NULL; char* DATADST_DIR = alloca(strlen(get_tmpdir())+strlen(DATADST_DIR_SUFFIX)+1); sprintf(DATADST_DIR, "%s%s", get_tmpdir(), DATADST_DIR_SUFFIX); @@ -153,7 +181,7 @@ static int install_archive_image(struct img_type *img, if (strlen(img->path) == 0) { TRACE("Missing path attribute"); - return -1; + goto out; } if (use_mount) { @@ -161,31 +189,37 @@ static int install_archive_image(struct img_type *img, if (ret) { ERROR("Device %s with filesystem %s cannot be mounted", img->device, img->filesystem); - return -1; + goto out; } + is_mounted = 1; + if (snprintf(path, sizeof(path), "%s%s", DATADST_DIR, img->path) >= (int)sizeof(path)) { ERROR("Path too long: %s%s", DATADST_DIR, img->path); - return -1; + goto out; } } else { if (snprintf(path, sizeof(path), "%s", img->path) >= (int)sizeof(path)) { ERROR("Path too long: %s", img->path); - return -1; + goto out; } } - char* FIFO = alloca(strlen(get_tmpdir())+strlen(FIFO_FILE_NAME)+1); + FIFO = alloca(strlen(get_tmpdir())+strlen(FIFO_FILE_NAME)+1); sprintf(FIFO, "%s%s", get_tmpdir(), FIFO_FILE_NAME); unlink(FIFO); - ret = mkfifo(FIFO, 0666); + ret = mkfifo(FIFO, 0660); if (ret) { ERROR("FIFO cannot be created in archive handler"); - return -1; + goto out; + } + + if (!getcwd(pwd, sizeof(pwd))) { + ERROR("Failed to determine current working directory"); + pwd[0] = '\0'; + goto out; } - if (!getcwd(pwd, sizeof(pwd))) - return -1; /* * Change to directory where tarball must be extracted @@ -193,7 +227,7 @@ static int install_archive_image(struct img_type *img, ret = chdir(path); if (ret) { ERROR("Fault: chdir not possible"); - return -EFAULT; + goto out; } TRACE("Installing file %s on %s, %s attributes", @@ -208,42 +242,68 @@ static int install_archive_image(struct img_type *img, ARCHIVE_EXTRACT_FFLAGS | ARCHIVE_EXTRACT_XATTR; } - ret = pthread_create(&extract_thread, &attr, extract, &tf); - if (ret) { + thread_ret = pthread_create(&extract_thread, &attr, extract, &tf); + if (thread_ret) { ERROR("Code from pthread_create() is %d", - ret); - return -EFAULT; + thread_ret); + goto out; } fdout = open(FIFO, O_WRONLY); + if (fdout < 0) { + ERROR("Failed to open FIFO %s", FIFO); + goto out; + } ret = copyimage(&fdout, img, NULL); - if (ret< 0) { + if (ret < 0) { ERROR("Error copying extracted file"); - return -EFAULT; + goto out; } - close(fdout); + exitval = 0; - ret = pthread_join(extract_thread, &status); - if (ret) { - ERROR("return code from pthread_join() is %d", ret); - return -EFAULT; +out: + if (fdout >= 0) { + ret = close(fdout); + if (ret) { + ERROR("failed to close FIFO %s", FIFO); + exitval = -EFAULT; + } } - unlink(FIFO); - - ret = chdir(pwd); + if (thread_ret == 0) { + ret = pthread_join(extract_thread, &status); + if (ret) { + ERROR("return code from pthread_join() is %d", ret); + exitval = -EFAULT; + } + else if ((int)status != 0) { + ERROR("copyimage status code is %d", (int)status); + exitval = -EFAULT; + } + } - if (ret) { - TRACE("Fault: chdir not possible"); + if (pwd[0] != '\0') { + ret = chdir(pwd); + if (ret) { + ERROR("chdir failed to revert to directory %s", pwd); + exitval = -EFAULT; + } } - if (use_mount) { - swupdate_umount(DATADST_DIR); + if (FIFO != NULL) + unlink(FIFO); + + if (is_mounted) { + ret = swupdate_umount(DATADST_DIR); + if (ret) { + ERROR("Failed to unmount directory %s", DATADST_DIR); + exitval = -EFAULT; + } } - return ret; + return exitval; } __attribute__((constructor))