From patchwork Wed Apr 24 02:57:25 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: 1089857 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::d39; helo=mail-io1-xd39.google.com; envelope-from=swupdate+bncbcg4lpfp6aorbqnb77sqkgqedmxjzdi@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="nwSLyop9"; dkim-atps=neutral Received: from mail-io1-xd39.google.com (mail-io1-xd39.google.com [IPv6:2607:f8b0:4864:20::d39]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 44plPL5VcSz9s4V for ; Wed, 24 Apr 2019 12:58:12 +1000 (AEST) Received: by mail-io1-xd39.google.com with SMTP id y5sf6423949ioc.22 for ; Tue, 23 Apr 2019 19:58:12 -0700 (PDT) ARC-Seal: i=2; a=rsa-sha256; t=1556074689; cv=pass; d=google.com; s=arc-20160816; b=gHIYPQPLbdg/FvOE+zTFdAb4klXC72R5Nw53TUTNGy1qCrPNK2zkQcwRKcEGcyx+xa BcM8Wq/4Lvj3UySSQjfXroNfwzl5z/VCIPO0bd9w0NxT8l/CYPRSc7IMOTEyS8xwkOj9 2JcHyrMENLn6uxhqaEDF1AasvNtEg/uF7lLE6+1C4dme+OabnKcYRZGkmwEmdfl6FRkq aKxwPKnvXhsgfD9mMnm9KFBed6YaI4q4vEB9cxFfF3Psy9t0uMvTywJ2CNOxrwogz0xl gibeR51KQtv2mYX6Wp5Rf04quJnO+kK2l+xiCYWxUdc8D3U7kWLOIRg7BjHYmXVeZCbM fRHA== 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=AadKANCyVI/3MSjoJDeV7TIZF9+9/RynZLIdsAwbDLw=; b=mvC4cwGtMnZFijvWRQcK7m55C6VOqINJgig8RxFQZE+ccMEkio1Via6XFhhePhm3P6 YE3uCufTOxZyeQIgrrAGUyEEe05paZRNhogE0eTp9tEqM1VmXBxFO9ENEJORKXQPcGuE ZmGr1XY8wONBYnmw1lwfgcImPwXyJq8O0nONVD1qdIzhgCwLtnoOpkVy/BoIEkdbpJTx B+hMK+WjOcj0luZlZPfGXRTqYhu9TDSBGnPGxDuVZHfxKMu+NC/QQ+12WQt2lD1opsgV XFZoMYqK4IG5+OTFvXHgaaZ9MpHq6drnxCIxWLjgSmzb4Ox5AwxO9PPnxWWnaZnstf3Q 8qrQ== ARC-Authentication-Results: i=2; gmr-mx.google.com; dkim=pass header.i=@planetinnovation.com.au header.s=google header.b=atdI3Es9; spf=pass (google.com: domain of austin.phillips@planetinnovation.com.au designates 2607:f8b0:4864:20::444 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=AadKANCyVI/3MSjoJDeV7TIZF9+9/RynZLIdsAwbDLw=; b=nwSLyop9TdWp1rGqeQxp4Yg59Uz+6wAP/1yQ++N0S7+r6dcCaNzF5Pw8yP41VIoZQO W4QUJOByufvdqth6ITpT+3btVFIloVWpeyhGf8xOALT0+eCvaKpRse/4//FF8+UNH1mo pZDHObD+SV0H2lnC0o0jpU1IIydL0P6J7hYXBdMkf4vz4ZMh7dmWaM0I32CcDhLj17mu CLnUHIC4/Vnj4br2+9JGJ5uFYuqGUW1trfhzLa1x7frGxBJsJn/HqFpiV1Q1DgSkfxEs Bf5Rj1/5PkX9CdF1pWbaSS1GMnFgDPEGt5EQpm/OxVSaHl+u31X+9ksMOR8IUmQbWdZb IKXg== 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=AadKANCyVI/3MSjoJDeV7TIZF9+9/RynZLIdsAwbDLw=; b=SHxKqi5B7htBq7XOezDAzNiv4gSFnjY/mZopLGCFtFdNJzHnZi82cxdnlUFDd32k2A mgHkzlKMfOkDEp3T+Ha4Y8mh9soOVnDi6DiVQ/nRrPGQQzxgKTtdoQCYSYuo22CEd+zN t7tDVm000rH6Uo6FyLTikVhBD8RGicBRD1U7GM9NXU5jPW+qnPG2538UnOGkiy3/9Ott YWBeFu9Gkpf3HwKlslJv+RrFQEXbUO4BYhAbtPIdkUnQMntrOqbiCBkXXEccbvCb19Re Rp9cRW42eb2utG+CjpAqoOeNuC+Phpw3FCaKzdSOw/FTmfHe4pORSe0IlHOj9ppInSla jdpQ== X-Gm-Message-State: APjAAAVGerDoIi/E6nZr0QoLF4NlybevXcohgAhvYcBLHK7/CVZm8uG9 PCaWLx5MbbKVpA0opoPuuCo= X-Google-Smtp-Source: APXvYqxlzpB+X9g8OtVtTtlfbPjUKbGV06XSsl+GT1XkENvEi01aaBzYlvnSR2mdC6KEg+fkr2Jwfg== X-Received: by 2002:a6b:c9cb:: with SMTP id z194mr1117229iof.244.1556074689598; Tue, 23 Apr 2019 19:58:09 -0700 (PDT) X-BeenThere: swupdate@googlegroups.com Received: by 2002:a02:8384:: with SMTP id z4ls1424549jag.16.gmail; Tue, 23 Apr 2019 19:58:08 -0700 (PDT) X-Received: by 2002:a02:3f0c:: with SMTP id d12mr14099722jaa.9.1556074688935; Tue, 23 Apr 2019 19:58:08 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1556074688; cv=none; d=google.com; s=arc-20160816; b=OTCCtPVKQMmYElYeA7HoiBj4GMd8fI1x/LctsdwdsqtZXUF0tjOwEVUL/pwLGrm5Ej wkUAcAhf2fnbz+QwhNqMWbs3dWEq3nc3irX8zlB0dtu2/y6iKSIjdPG8oOLX1o5X1x8i EA/LC2r5dP3Fr6MITRGKQ907MCGh01r20UaVACAYChsAZ9Gs4ZHD4jH/NThtaPRVzOj0 bWU8VP0TrjANSvNeKRNBASe7KEkhos6g2vhJ7nAZHymAIY6/KHRASjzc7SfaoOUglRDH oMevwnf3nYW+LbnhfBlOvgGla7WRnb4QwRreHe05kmyojXGnaSSKQA2TPa6MPItC2APD Ovyg== 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=5t5DgVagAkYcmmdAi6UIVqO8wKYVUE10EIrbSYuCSso=; b=cBYuZs3WrOn9bxt4zZmzKG/dP/Gq4gUYaH6T344Ku/ZyoUhRnEqYmSeFqb8+N+l7c0 UkvVBdEtl1l0436M38egouhIusscb3meyWVso0hJMN+iK3PAmC/3zFRWn+b1AoOTgYUU Hv5JZCIJOu3KABOusqayV14LyQis6ryZBIkvnbvM/Wg9sF8jr953VaTs1X8VHqxuXGzI JvQ8WGGqxNQnzfRlQRrTpgoDXwlF15nQ6OmyU7fuGqnRCP4dIsKPIQtE2mApMJDhdbG1 YYZ3LHVmXQ/yDUPoFAn59K8sMUK082EZ9Sp8iYzWvzwc66zbfsU6A3TytWonQ6oJkT1U sVgg== ARC-Authentication-Results: i=1; gmr-mx.google.com; dkim=pass header.i=@planetinnovation.com.au header.s=google header.b=atdI3Es9; spf=pass (google.com: domain of austin.phillips@planetinnovation.com.au designates 2607:f8b0:4864:20::444 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-pf1-x444.google.com (mail-pf1-x444.google.com. [2607:f8b0:4864:20::444]) by gmr-mx.google.com with ESMTPS id d74si1452045itd.3.2019.04.23.19.58.08 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 23 Apr 2019 19:58:08 -0700 (PDT) Received-SPF: pass (google.com: domain of austin.phillips@planetinnovation.com.au designates 2607:f8b0:4864:20::444 as permitted sender) client-ip=2607:f8b0:4864:20::444; Received: by mail-pf1-x444.google.com with SMTP id e67so637254pfe.10 for ; Tue, 23 Apr 2019 19:58:08 -0700 (PDT) MIME-Version: 1.0 X-Received: by 2002:aa7:8251:: with SMTP id e17mr31740824pfn.147.1556074687907; Tue, 23 Apr 2019 19:58:07 -0700 (PDT) Received: from localhost.localdomain (gen-119-17-166-139.ptr4.otw.net.au. [119.17.166.139]) by smtp.googlemail.com with ESMTPSA id f7sm22754919pga.56.2019.04.23.19.58.05 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Tue, 23 Apr 2019 19:58:06 -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 v3] handlers: Improve archive_handler error handling Date: Wed, 24 Apr 2019 12:57:25 +1000 Message-Id: <1556074645-37825-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=atdI3Es9; spf=pass (google.com: domain of austin.phillips@planetinnovation.com.au designates 2607:f8b0:4864:20::444 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 0600 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 | 138 ++++++++++++++++++++++++++++++++------------- 1 file changed, 99 insertions(+), 39 deletions(-) diff --git a/handlers/archive_handler.c b/handlers/archive_handler.c index a44819d..7f12761 100644 --- a/handlers/archive_handler.c +++ b/handlers/archive_handler.c @@ -35,6 +35,7 @@ pthread_t extract_thread; struct extract_data { int flags; + int exitval; }; static int @@ -68,15 +69,24 @@ static void * extract(void *p) { struct archive *a; - struct archive *ext; - struct archive_entry *entry; + 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) { + goto out; + } + ext = archive_write_disk_new(); + if (!ext) { + goto out; + } + archive_write_disk_set_options(ext, flags); /* * Note: archive_write_disk_set_standard_lookup() is useful @@ -97,7 +107,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 +116,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,32 +132,53 @@ 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) { + r = archive_write_free(ext); + if (r) { + ERROR("archive_write_free(): %s %d", + archive_error_string(a), r); + exitval = -EFAULT; + } + } + + if (a) { + archive_read_close(a); + archive_read_free(a); + } + + data->exitval = exitval; + pthread_exit(NULL); } 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* DATADST_DIR = alloca(strlen(get_tmpdir())+strlen(DATADST_DIR_SUFFIX)+1); sprintf(DATADST_DIR, "%s%s", get_tmpdir(), DATADST_DIR_SUFFIX); + char * FIFO = alloca(strlen(get_tmpdir())+strlen(FIFO_FILE_NAME)+1); + sprintf(FIFO, "%s%s", get_tmpdir(), FIFO_FILE_NAME); + pthread_attr_init(&attr); pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_JOINABLE); @@ -164,28 +195,32 @@ static int install_archive_image(struct img_type *img, return -1; } + 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); - sprintf(FIFO, "%s%s", get_tmpdir(), FIFO_FILE_NAME); unlink(FIFO); - ret = mkfifo(FIFO, 0666); + ret = mkfifo(FIFO, 0600); 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 +228,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", @@ -201,6 +236,7 @@ static int install_archive_image(struct img_type *img, img->preserve_attributes ? "preserving" : "ignoring"); tf.flags = 0; + tf.exitval = -EFAULT; if (img->preserve_attributes) { tf.flags |= ARCHIVE_EXTRACT_OWNER | ARCHIVE_EXTRACT_PERM | @@ -208,42 +244,66 @@ 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); + } } - unlink(FIFO); + if (!thread_ret) { + void *status; - ret = chdir(pwd); + ret = pthread_join(extract_thread, &status); + if (ret) { + ERROR("return code from pthread_join() is %d", ret); + exitval = -EFAULT; + } + else if (tf.exitval != 0) { + ERROR("copyimage status code is %d", tf.exitval); + exitval = -EFAULT; + } + } - if (ret) { - TRACE("Fault: chdir not possible"); + if (pwd[0]) { + ret = chdir(pwd); + if (ret) { + ERROR("chdir failed to revert to directory %s", pwd); + } } - if (use_mount) { - swupdate_umount(DATADST_DIR); + unlink(FIFO); + + if (is_mounted) { + ret = swupdate_umount(DATADST_DIR); + if (ret) { + TRACE("Failed to unmount directory %s", DATADST_DIR); + } } - return ret; + return exitval; } __attribute__((constructor))