Use libarchive instead of calling out to tar #327

matthewrsj · 2017-10-31T20:42:46Z

Use libarchive in order to make use of its security features and avoid
calling out to tar via a shell. The TAR_COMMAND is still used in
staging.c to complete the copy when a hardlink fails.

Signed-off-by: Matthew Johnson matthew.johnson@intel.com

icarus-sparry · 2017-10-31T20:53:25Z

src/archives.c

+
+		r = archive_write_header(ext, entry);
+		if (r < ARCHIVE_OK) {
+			fprintf(stderr, "%s\n", archive_error_string(ext));


Error handling looks wrong here, We get an error from archive_write_header, which we print out, skip the copying, then call archive_write_finish_entry and carry on....

Looks like it is missing a if (r < ARCHIVE_WARN) return r;

icarus-sparry · 2017-10-31T20:57:57Z

src/archives.c

+		}
+	}
+
+	archive_read_close(a);


This looks like resource leaks whenever we do a 'return r' above.

yep, I'll fix that.

icarus-sparry · 2017-10-31T21:05:48Z

src/archives.c

+	}
+}
+
+int extract(const char *filename, const char *target)


At the very least this need some documentation to say what the parameters are, and that passing NULL as target is valid (but magic).

True, I'll add a docstring

Not a fan of the name "extract", given it has two parameters, the tarfile and the directory to unpack into. Perhaps "untarto". I would expect that something called "extract" would have the name of the thing to extract as a parameter.

ok, I'm thinking extract_to(...) then

icarus-sparry · 2017-10-31T21:07:32Z

src/archives.c

+			fprintf(stderr, "%s\n", archive_error_string(ext));
+		} else if (archive_entry_size(entry) > 0) {
+			r = copy_data(a, ext);
+			if (r < ARCHIVE_OK) {


copy_data already prints an error in the write case. Probably should make copy_data silent and leave this message.

matthewrsj · 2017-10-31T21:13:05Z

Having trouble getting libarchive installed in the Travis environment.

icarus-sparry · 2017-10-31T21:23:35Z

src/archives.c

+		r = archive_write_header(ext, entry);
+		if (r < ARCHIVE_OK) {
+			fprintf(stderr, "%s\n", archive_error_string(ext));
+		} else if (archive_entry_size(entry) > 0) {


Does this code, which skips copy_data for zero length mean we can't create zero length files?

Quick test shows me we can create zero length files. From the man page:

The function archive_entry_size() returns the file size, if it has been set, and 0 otherwise. archive_entry_size() can be used to query that status.

When I created an archive with several empty files I was able to extract them using this code.

matthewrsj · 2017-10-31T23:22:27Z

*closed by mistake, oops

Fixes #285

icarus-sparry

+1

tmarcu · 2017-11-01T00:04:19Z

src/archives.c

+	int r;
+	const void *buffer;
+	size_t size;
+	la_int64_t offset;


Just to be pedantic, from the man pages the function below is defined archive_read_data_block(struct archive *, const void **buff, size_t *len, off_t *offset);
I know off_t 'cause it's a standard type, so it would make sense to match them up and make it: off_t offset IMHO, then all the types are obvious and matching.

tmarcu · 2017-11-01T00:49:41Z

+1

matthewrsj · 2017-11-01T18:25:42Z

Resolves #280 as well.

phmccarty · 2017-11-01T22:16:40Z

src/download.c

+	char *outputdir;
+	string_or_die(&outputdir, "%s/staged", state_dir);
+	err = extract_to(tarfile, outputdir);
+	free(outputdir);


There is a large FIXME in the if block below that relates to using GNU tar. That should probably be updated to reflect the new use of libarchive, or removed if it's no longer relevant.

Updated, other comments addressed as well.

phmccarty · 2017-11-01T22:23:55Z

src/archives.c

+	/* set up read */
+	a = archive_read_new();
+	archive_read_support_format_all(a);
+	archive_read_support_filter_all(a);


These two functions return int, so consider adding error handling if appropriate, or cast to void if they should be ignored.

phmccarty · 2017-11-01T22:25:17Z

src/archives.c

+	/* set up write */
+	ext = archive_write_disk_new();
+	archive_write_disk_set_options(ext, flags);
+	archive_write_disk_set_standard_lookup(ext);


Similar comments for these two functions as for archive_read_*.

they can technically fail with ARCHIVE_FATAL, so might as well check that since I have no idea how often that can happen.

phmccarty · 2017-11-01T22:26:31Z

src/archives.c

+	r = archive_read_open_filename(a, tarfile, 10240);
+	if (r) {
+		/* could not open archive for read */
+		fprintf(stderr, "%s\n", archive_error_string(a));


Maybe prefix the string with Error: ? Unless libarchive does that already.

phmccarty · 2017-11-01T22:30:53Z

src/archives.c

+/* archive_check_err(ar, ret)
+ *
+ * Check and print archive errors */
+static int archive_check_err(struct archive *ar, int ret)


This function name confused me at first because I thought it was part of libarchive. I would prefer a different name, or at least a leading underscore.

phmccarty · 2017-11-01T22:35:36Z

src/archives.c

+	}
+
+	/* just a warning */
+	return 0;


Is any special handling needed for ARCHIVE_RETRY?

Good catch, since ARCHIVE_RETRY indicates the operation failed, but is -10 while ARCHIVE_WARN is -20, so would not be caught by this.

I think we should just treat ARCHIVE_RETRY as an extraction failure rather than trying to figure out how many times we want to continue retrying for each separate operation.

That sounds fine to me. Implementing extraction retries based on this status code sounds overkill.

phmccarty · 2017-11-01T22:37:06Z

src/archives.c

+	/* set which attributes we want to restore */
+	flags = ARCHIVE_EXTRACT_TIME;
+	flags |= ARCHIVE_EXTRACT_PERM;
+	flags |= ARCHIVE_EXTRACT_ACL;


I'm wondering why this flag is set. swupd doesn't handle extended ACLs (afaik).

phmccarty · 2017-11-01T22:38:38Z

src/archives.c

+	flags = ARCHIVE_EXTRACT_TIME;
+	flags |= ARCHIVE_EXTRACT_PERM;
+	flags |= ARCHIVE_EXTRACT_ACL;
+	flags |= ARCHIVE_EXTRACT_FFLAGS;


I'm also not sure about this flag... doesn't look relevant for swupd.

I took the flags the libarchive documentation (on their wiki) provided as an example to preserve file attributes. I'll look at the flags closer and modify.

phmccarty · 2017-11-01T22:40:37Z

src/archives.c

+	flags |= ARCHIVE_EXTRACT_PERM;
+	flags |= ARCHIVE_EXTRACT_ACL;
+	flags |= ARCHIVE_EXTRACT_FFLAGS;
+	flags |= ARCHIVE_EXTRACT_XATTR;


Looking through the flag list, I think ARCHIVE_EXTRACT_OWNER is notably missing.

phmccarty · 2017-11-01T23:31:45Z

src/hash.c

-			string_or_die(&tar, TAR_COMMAND " -C %s/%i -xf %s/%i/Manifest.%s.tar 2> /dev/null",
-				      state_dir, current->last_change, state_dir,
-				      current->last_change, current->filename);
+			ret = extract_to(filename, state_dir);


The manifest tars live in the versioned directories under statedir, not directly under statedir.

pohly · 2017-11-16T15:55:29Z

src/archives.c

+
+	/* set up read */
+	a = archive_read_new();
+	r = archive_read_support_format_all(a);


You can save some CPU cycles by limiting the format to what will actually be present in the archive, i.e. tar.

ah, you're right. I'll do that.

Ah, I thought it was deliberate that you were being flexible.

the flexible part is archive_read_support_filter_all, so we can support whatever compression is sent to it.

pohly · 2017-11-17T09:20:50Z

src/archives.c

+	}
+
+out:
+	archive_write_close(ext);


There's no error handling here. archive_write_close() sets deferred directory attributes, so it can still fail.

What should we be doing if this fails? Just set r and continue trying to free the write and close and free the read? That way the calling function will see the error but we will still be attempting to clean up after ourselves.

pohly · 2017-11-20T11:23:48Z

On Fri, 2017-11-17 at 20:10 +0000, Matthew Johnson wrote

What should we be doing if this fails? Just set r and continue trying to free the write and close and free the read?

Yes. Basically untarring has failed and the caller needs to be alerted about that.

phmccarty

I reviewed the libarchive usage here, and everything looks fine to me. Just one comment regarding the new copyright header.

phmccarty · 2017-11-30T23:48:58Z

src/archives.c

+/*
+ *   Software Updater - client side
+ *
+ *      Copyright © 2012-2017 Intel Corporation.


Use libarchive in order to make use of its security features and avoid calling out to tar via a shell. The TAR_COMMAND is still used in staging.c to complete the copy when a hardlink fails. Signed-off-by: Matthew Johnson <matthew.johnson@intel.com>

Signed-off-by: Matthew Johnson <matthew.johnson@intel.com>

matthewrsj force-pushed the libarchive branch from 338034d to c6e4493 Compare October 31, 2017 20:43

icarus-sparry reviewed Oct 31, 2017

View reviewed changes

matthewrsj force-pushed the libarchive branch 4 times, most recently from c32398b to ec14482 Compare October 31, 2017 23:17

matthewrsj closed this Oct 31, 2017

matthewrsj reopened this Oct 31, 2017

matthewrsj force-pushed the libarchive branch from e808996 to 2670483 Compare October 31, 2017 23:32

icarus-sparry approved these changes Oct 31, 2017

View reviewed changes

tmarcu reviewed Nov 1, 2017

View reviewed changes

matthewrsj force-pushed the libarchive branch from 2670483 to a66fe34 Compare November 1, 2017 00:09

This was referenced Nov 1, 2017

download required files via HTTP on demand clearlinux/swupd-server#47

Closed

bsdtar: ignore UTF-8 encoding errors #280

Closed

phmccarty reviewed Nov 1, 2017

View reviewed changes

matthewrsj force-pushed the libarchive branch from a66fe34 to 3d22e9b Compare November 2, 2017 21:07

pohly reviewed Nov 16, 2017

View reviewed changes

matthewrsj force-pushed the libarchive branch from 3d22e9b to dc8258d Compare November 17, 2017 00:11

matthewrsj requested a review from phmccarty November 17, 2017 00:12

pohly reviewed Nov 17, 2017

View reviewed changes

matthewrsj force-pushed the libarchive branch from dc8258d to d88f889 Compare November 20, 2017 17:41

phmccarty reviewed Dec 1, 2017

View reviewed changes

matthewrsj added 2 commits November 30, 2017 16:53

Add travis configuration for libarchive

8c373d5

Signed-off-by: Matthew Johnson <matthew.johnson@intel.com>

matthewrsj force-pushed the libarchive branch from d88f889 to 8c373d5 Compare December 1, 2017 00:58

matthewrsj merged commit 08f8614 into clearlinux:master Dec 1, 2017

matthewrsj deleted the libarchive branch December 1, 2017 01:15

matthewrsj mentioned this pull request Jan 19, 2018

use libarchive directly #285

Closed

Use libarchive instead of calling out to tar #327

Use libarchive instead of calling out to tar #327

Conversation

matthewrsj commented Oct 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewrsj commented Oct 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewrsj commented Oct 31, 2017

icarus-sparry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmarcu commented Nov 1, 2017

matthewrsj commented Nov 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phmccarty Nov 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewrsj Nov 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pohly commented Nov 20, 2017 via email

phmccarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phmccarty Nov 1, 2017 •

edited

matthewrsj Nov 16, 2017 •

edited