From patchwork Tue Apr 2 00:12:09 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: 1073534 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::c3a; helo=mail-yw1-xc3a.google.com; envelope-from=swupdate+bncbcg4lpfp6aorb7wrrlsqkgqepfsrnpq@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="BSesnOWE"; dkim-atps=neutral Received: from mail-yw1-xc3a.google.com (mail-yw1-xc3a.google.com [IPv6:2607:f8b0:4864:20::c3a]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 44Y8md3DqDz9sSj for ; Tue, 2 Apr 2019 11:12:49 +1100 (AEDT) Received: by mail-yw1-xc3a.google.com with SMTP id v123sf8678582ywf.16 for ; Mon, 01 Apr 2019 17:12:49 -0700 (PDT) ARC-Seal: i=2; a=rsa-sha256; t=1554163966; cv=pass; d=google.com; s=arc-20160816; b=wVxUxsATPT+0YvyLcikPe1l4XHfX1VsczeqbcwBcOVef37Y8rd25VlsOaFuw0sAPkU BIzvllWtQ/b2Jruhgs/E5XgTBrEUbEpBelB/1FqXA6C/ORwz/rXqyXnX92GcURC8ZQZ5 XFCsoUqqJV/LJQxqNhO0bC/QIfcF9G9M241O+9D114RGTVwyPcQDIdhwbi20vwf9uWev EXALqT3rxPB9tK9d0dSIkLG+k0kmgYuTlkdpoZ/7l4g7y3tUsJqdP7n1s01IzXRw8zE/ 8hhiirqHbHBxewvKPTEhD57Kx0R6mmHsE9y7LPK1rKdnmTDI7eQeHBu76ifPdWkCejiC 1dlg== 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=3sC2q3ofQCiq8K1+CFrLUYkqQtoGf1lhzw7BrRVTG54=; b=JJmrowgH7KPAs9WM8qPc8//ECBd+7fEVDg9GMauqPO+wYUJ+RLhq0sEpFh+FdVNRFj e/HHc8sSFDhdwYqwPIu6P/NC44HdA9ylkaI1jTB/XoAufDRoNMVc1N+IIVuEkAbzLn17 QZyP/0INlMBhSqVcgaNuPiMWoacYqQ6exqrMTewCtAY/UvGuy3K0CDFlE//KLlIF0Vcq FX8eppMV7xfokNzXwMEtd3CrE8aGtvWbedXERlzMoXv2n5tgmEcFIFmky0GZbsaVE8LP 1+ieuQhVhhvErkj5jiH/VryluQTvEIRvMngdNVP50HRkOCyG+ljy0Ia7NpNBh+941Vus QtHg== ARC-Authentication-Results: i=2; gmr-mx.google.com; dkim=pass header.i=@planetinnovation.com.au header.s=google header.b=E2JPfvvi; spf=pass (google.com: domain of austin.phillips@planetinnovation.com.au designates 2607:f8b0:4864:20::542 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=3sC2q3ofQCiq8K1+CFrLUYkqQtoGf1lhzw7BrRVTG54=; b=BSesnOWElvDkRQNHJ7MyY0yoIRnoFuvG+Qtbe1gvQeHHtmknQeRXAbXu9bubokmoB0 eqKc+WRo82gh+mS2uHIi81Zre4SqJstQU87voYn4Pb59ACV4J+KfJVKPLO1wAsCMnIeM 0clKD1GEv/k3HAznZhOSCY3V1gW/+bXBUmAeZjK11k26XPh02+de9fGSjBpNy2GOYnIz XFC9YVu+z6rnnPozi+LK0yRj0fqALZtPM1y4it1LZESvxBi6KBZ6hiFzbzj/mc/Spmdc HP3XzPIaRb62CRbkEL1VjDW7Wk2dIh9QqIPmC177HhozGhvRlJgjnrFGN2/drPG0TCSf 4cGw== 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=3sC2q3ofQCiq8K1+CFrLUYkqQtoGf1lhzw7BrRVTG54=; b=Yhxf9tIJPy0ZERFnN5fz0KTOj75ka6xrWEppML8L083axS9f3TZu8VZpHcUhT/ZYhU zrDBFcHDsK1Z+nu3uXaDhGSS4dWl6aqREKBGvbFJeGeXsro+KylFTzSshcG39NpZLA6s CgEO8DoO4u1mXki4kBKrPFV+oIP732fv0WcEukTHRYzDiezgIUwkmdK0odigNl5q2wPP plPU4j/gdyJ7kZpajDq9mv8vp/Vx8eUSkGRx3/j3Q0yjBh93kOw9wpO3nH/wP1RCG/SS 2WbecJ/1xHW/te0wLPebmMooGN+JsNvzFpM8VwT7XUHM31RNITLeO+juzbx8XBfG1yMQ 7mZw== X-Gm-Message-State: APjAAAUmZU+bdB6/oPVJIus76iiJPvTsaR8i+MgE2Co2a0hRU1wCUbja 2mwuPOxy9Ylvp8dVkru3/xg= X-Google-Smtp-Source: APXvYqxtXXCn1AppSnemVNyXfTDSwl4F8GmzBsKJKDoSQxDokkq/rYQdn2S96SxBLaJDD+6Q15n17g== X-Received: by 2002:a0d:db91:: with SMTP id d139mr55763909ywe.418.1554163966365; Mon, 01 Apr 2019 17:12:46 -0700 (PDT) X-BeenThere: swupdate@googlegroups.com Received: by 2002:a25:4c4:: with SMTP id 187ls330191ybe.11.gmail; Mon, 01 Apr 2019 17:12:45 -0700 (PDT) X-Received: by 2002:a25:ca85:: with SMTP id a127mr677657ybg.24.1554163965887; Mon, 01 Apr 2019 17:12:45 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1554163965; cv=none; d=google.com; s=arc-20160816; b=mwmtIUX5M8nhigXc88zET8og1NFqGU760eLd+IA4hxlv/GID601kfs6hcoZ+yJg5KY CWrxuaL7+aiOCO1lhNdPJ127wAJQvHXO2mMc5++sMEF7I92X0JlpHJ8LkAr9LvcHYNlq UQXwbnTB7cR56FHeWy8G9JNL8o6XQZU7SDkzQg4Ql598k5eWZzG7cNPgvnJp7zHmQJeF Ob1bk9bp7f+OSizDRHoo+1dcN0bvT/Rrm4luBS1D95riB/hYNwzuW9r7sZ+skeP+LJW2 iCZd7pc3V3uzMKmoSeTTteRjwOkI3NHcPROhzSY7TQCgs07waiqep7y9lWcGyQblvVL/ STHA== 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=6Biz4BFTXyO4ATqU/+g2Ag5R3S/1e3tLUjSaMGv6d1E=; b=W131ogsEGMmTBEG40tKtLDGMu0tobhHZq692MRhKD+hAzGcH94VrXVMoN8FyfmgsSD HKmB2S2FqnB+QjYkXjCqBhEu+s5K9GcT4X8Jdy7JHSDn9UCyumcaQLzogtUXO6gJo/ZY HDufH6PlBbgYLpF542PvYYHqIkJXwAICYfBQLMhXVvXQabkKL9D+fkCvH/ay2YRyNLRx P/8iFkzq1pBmhqsGXn0gvl8viJDcll5WDn9kvognJXjiOuhn7B6ZnaMOzXd5RUR4HTRO A74GG5Tvjf4z7IYVScg4ulmGAl5ROSeiENXtdpg9sVw2eBJvd4MwkKp8v/ulNwBbwW3s E3oA== ARC-Authentication-Results: i=1; gmr-mx.google.com; dkim=pass header.i=@planetinnovation.com.au header.s=google header.b=E2JPfvvi; spf=pass (google.com: domain of austin.phillips@planetinnovation.com.au designates 2607:f8b0:4864:20::542 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-x542.google.com (mail-pg1-x542.google.com. [2607:f8b0:4864:20::542]) by gmr-mx.google.com with ESMTPS id k11si559807ybf.0.2019.04.01.17.12.45 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 01 Apr 2019 17:12:45 -0700 (PDT) Received-SPF: pass (google.com: domain of austin.phillips@planetinnovation.com.au designates 2607:f8b0:4864:20::542 as permitted sender) client-ip=2607:f8b0:4864:20::542; Received: by mail-pg1-x542.google.com with SMTP id b12so5604681pgk.0 for ; Mon, 01 Apr 2019 17:12:45 -0700 (PDT) MIME-Version: 1.0 X-Received: by 2002:a63:3d4c:: with SMTP id k73mr47144072pga.154.1554163964802; Mon, 01 Apr 2019 17:12:44 -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 w189sm4313251pfw.147.2019.04.01.17.12.42 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Mon, 01 Apr 2019 17:12: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 V2] handlers: Improve archive_handler error handling Date: Tue, 2 Apr 2019 11:12:09 +1100 Message-Id: <1554163929-114356-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=E2JPfvvi; spf=pass (google.com: domain of austin.phillips@planetinnovation.com.au designates 2607:f8b0:4864:20::542 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 | 134 ++++++++++++++++++++++++++++++++------------- 1 file changed, 95 insertions(+), 39 deletions(-) diff --git a/handlers/archive_handler.c b/handlers/archive_handler.c index a44819d..49c5cad 100644 --- a/handlers/archive_handler.c +++ b/handlers/archive_handler.c @@ -68,15 +68,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 +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,32 +131,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); + } + + 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* 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 +194,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 +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,64 @@ 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); - - ret = chdir(pwd); + if (!thread_ret) { + 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]) { + 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))