Skip to content
This repository has been archived by the owner on Jan 6, 2023. It is now read-only.

Double free corruption with alternative input layout #54

Open
tmarcu opened this issue Dec 1, 2016 · 6 comments
Open

Double free corruption with alternative input layout #54

tmarcu opened this issue Dec 1, 2016 · 6 comments

Comments

@tmarcu
Copy link
Contributor

tmarcu commented Dec 1, 2016

This patch (#45) causes a double free on two tests. Travis-ci is now enabled with the latest commits, so all the tests will be automatically run for following PRs to catch such regressions.

@pohly
Copy link
Contributor

pohly commented Dec 1, 2016

Do you have additional details about the double free? Was it detected by glibc or valgrind?

I've not seen that in my own testing. If it's in the code paths that only happen when building without the alternative input layout, then I some help tracking down the problem would be welcome.

@pohly
Copy link
Contributor

pohly commented Dec 1, 2016

Actually, trying the modified swupd-server with traditional meta-swupd (and thus the original input layout) won't be that hard to test for me - I'll try tomorrow and report back.

@phmccarty
Copy link
Contributor

phmccarty commented Dec 1, 2016

@pohly: @tmarcu and I were seeing the same two test failures, but they appeared to manifest for different reasons. My failures are below.

The "file-name-blacklisted" test fails with swupd_create_update: src/manifest.c:120: manifest_from_file: Assertion `0' failed..

The "state-file" test fails with a segfault. Backtrace:

#0  0x00007f842b0c626b in malloc_consolidate () from /usr/lib/libc.so.6
#1  0x00007f842b0c7d2a in _int_malloc () from /usr/lib/libc.so.6
#2  0x00007f842b0c9d44 in malloc () from /usr/lib/libc.so.6
#3  0x00007f842b0b5ab2 in __GI__IO_file_doallocate () from /usr/lib/libc.so.6
#4  0x00007f842b0c3ad6 in __GI__IO_doallocbuf () from /usr/lib/libc.so.6
#5  0x00007f842b0c2e38 in __GI__IO_file_overflow () from /usr/lib/libc.so.6
#6  0x00007f842b0c1f06 in __GI__IO_file_xsputn () from /usr/lib/libc.so.6
#7  0x00007f842b0b795d in __GI__IO_padn () from /usr/lib/libc.so.6
#8  0x00007f842b097e5e in vfprintf () from /usr/lib/libc.so.6
#9  0x00007f842b145ae9 in __fprintf_chk () from /usr/lib/libc.so.6
#10 0x0000000000407ba3 in fprintf (__fmt=0x40f5f0 "%3i.%03i %5s %s:%03i\t| %s\t| %s\t| %s\n", __stream=<optimized out>) at /usr/include/bits/stdio2.h:97
#11 __log_message (file=file@entry=0x0, msg=msg@entry=0x40f890 "Reading manifest", filename=filename@entry=0x40f6c8 "src/manifest.c", linenr=linenr@entry=126, fmt=fmt@entry=0x40d7f1 "%s") at src/log.c:125
#12 0x000000000040988a in manifest_from_file (version=0, component=component@entry=0x40f917 "full") at src/manifest.c:126
#13 0x0000000000402df4 in main (argc=<optimized out>, argv=<optimized out>) at src/create_update.c:316

pohly added a commit to pohly/swupd-server that referenced this issue Dec 2, 2016
In Ostro OS, we already have a "full" directory with all files.
Splitting it up into bundles just so that swupd-create-update can
reconstruct the "full" directory is a waste of IO, and noticably slow
when run under pseudo.

To streamline the required work, a new layout for the "image" input
directory gets introduced:
- The "full" directory gets created by the caller before invoking
  swupd-create-update.
- For each bundle, instead of a <bundle> directory, there is a
  <bundle>.content.txt file, listing all entries (including directories)
  of the bundle.

The traditional mode of operation still works as before because each operation
which normally works with a bundle directory checks whether there is such a
directory and if not, switches to the new mode.

That way it is even possible to mix the two modes, i.e. replacing only
some bundles with a content list, although that's probably not all
that useful.

This revised commit fixes the use of an uninitialized newversiondircontent
pointer in populate_dirs().

Fixes: swupd-server/clearlinux#54

Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
@pohly
Copy link
Contributor

pohly commented Dec 2, 2016

After setting up compilation without meta-swupd and running "make check", I was able to reproduce the failure. As usual with C, the root cause was a missing pointer initialization in that particular code path. Here's the fix:

diff --git a/src/create_update.c b/src/create_update.c
index cb7bf66..93d58d0 100644
--- a/src/create_update.c
+++ b/src/create_update.c
@@ -140,7 +140,7 @@ static bool parse_options(int argc, char **argv)
 static void populate_dirs(int version)
 {
        char *newversiondir;
-       char *newversiondircontent;
+       char *newversiondircontent = NULL;
 
        string_or_die(&newversiondir, "%s/%d", image_dir, version);
 

Please test and merge PR #55.

tmarcu pushed a commit that referenced this issue Dec 8, 2016
In Ostro OS, we already have a "full" directory with all files.
Splitting it up into bundles just so that swupd-create-update can
reconstruct the "full" directory is a waste of IO, and noticably slow
when run under pseudo.

To streamline the required work, a new layout for the "image" input
directory gets introduced:
- The "full" directory gets created by the caller before invoking
  swupd-create-update.
- For each bundle, instead of a <bundle> directory, there is a
  <bundle>.content.txt file, listing all entries (including directories)
  of the bundle.

The traditional mode of operation still works as before because each operation
which normally works with a bundle directory checks whether there is such a
directory and if not, switches to the new mode.

That way it is even possible to mix the two modes, i.e. replacing only
some bundles with a content list, although that's probably not all
that useful.

This revised commit fixes the use of an uninitialized newversiondircontent
pointer in populate_dirs().

Fixes: swupd-server/#54

Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
@matthewrsj
Copy link
Contributor

looks like this was resolved by #55 ?

@phmccarty
Copy link
Contributor

@matthewrsj Yes, I think so. We did not follow up on that PR yet.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants