Add squashfuse_extract tool #14

probonopd · 2016-10-09T19:25:08Z

The squashfuse_extract can extract files, directories, and symlinks from a squashfs image. Closes #13 and partly #9.

chipturner

Overall this looks nice, thanks for the contribution. I commented on a lot of minor issues, please feel free to fix them as you see fit, but the buffer overflow definitely should be addressed before landing.

chipturner · 2016-10-09T21:10:27Z

extract.c

+        die("sqfs_traverse_open error");
+    while (sqfs_traverse_next(&trv, &err)) {
+        if (!trv.dir_end) {
+            if((startsWith(path_to_extract, trv.path) != 0) || (strcmp("-a", path_to_extract) == 0)){


you could factor the second strcmp out into a one-time check, and then make this code a little clearer. also it might be nice to split this very long while/nested if block into separate functions or other factorings to make it a little easier to digest

Feel free to change as you see fit; for me it is OK as it is

chipturner · 2016-10-09T21:12:03Z

extract.c

+                fprintf(stderr, "inode.base.inode_type: %i\n", inode.base.inode_type);
+                fprintf(stderr, "inode.xtra.reg.file_size: %lu\n", inode.xtra.reg.file_size);
+                strcpy(prefixed_path_to_extract, "");
+                strcat(strcat(prefixed_path_to_extract, prefix), trv.path);


this is a potential buffer overflow; rather than a 1024 limit, you should allocate it dynamically once you know each trv.path (variable length array)

Thanks for pointing this out; fixed in b4618fc

chipturner · 2016-10-09T21:13:47Z

extract.c

+                strcpy(prefixed_path_to_extract, "");
+                strcat(strcat(prefixed_path_to_extract, prefix), trv.path);
+                if(inode.base.inode_type == SQUASHFS_DIR_TYPE){
+                    fprintf(stderr, "inode.xtra.dir.parent_inode: %ui\n", inode.xtra.dir.parent_inode);


consider putting these fprintf's into a debug mode and disable by default

I could but then I would have to implement fancier arg parsing - do you know how to do that without bloating the code?

chipturner · 2016-10-09T21:14:52Z

extract.c

+                        die("fopen error");
+                    while (bytes_already_read < inode.xtra.reg.file_size)
+                    {
+                        char *buf = malloc(bytes_at_a_time);


no need for explicit malloc/free -- you can use a VLA here, too

Changed in 8276a81

chipturner · 2016-10-09T21:15:22Z

extract.c

+                    fprintf(stderr, "Extract to: %s\n", prefixed_path_to_extract);
+                    // Read the file in chunks
+                    off_t bytes_already_read = 0;
+                    size_t bytes_at_a_time = 1024; 


I'd go with at least 64k, or up to 1mb; 1024 is pretty small and less efficient

Changed in b4618fc

chipturner · 2016-10-09T21:16:42Z

extract.c

+                    }
+                    fclose(f);
+                } else if(inode.base.inode_type == SQUASHFS_SYMLINK_TYPE){
+                    size_t size = 1024;


Changed in 8276a81

chipturner · 2016-10-09T21:19:52Z

extract.c

+                } else if(inode.base.inode_type == SQUASHFS_SYMLINK_TYPE){
+                    size_t size = 1024;
+                    char *buf = malloc(size);
+                    sqfs_readlink(&fs, &inode, buf, &size);


is there an error code you can check here?

Changed in 9e83e5d

chipturner · 2016-10-09T21:21:52Z

extract.c

+#define ERR_OPEN	(-3)
+
+static void usage() {
+    fprintf(stderr, "Usage: %s ARCHIVE PATH_TO_EXTRACT\n", PROGNAME);


a nice future enhancement would be to support extracting multiple files

Yes, but that's for a later version

chipturner · 2016-10-09T21:22:41Z

extract.c

+
+    prefix = "squashfs-root/";
+
+    if(access(prefix, F_OK ) == -1 ) {


style nit: space before the open paren of if statements; similar issue in other places

Changed in d743c1f

chipturner · 2016-10-09T21:23:10Z

extract.c

+
+#define PROGNAME "squashfuse_extract"
+
+#define ERR_MISC	(-1)


exit codes should be positive, not negative

Changed in 59ae332

probonopd · 2016-10-09T22:08:32Z

Thanks for your thorough review @chipturner. I have addressed your points. Now I would like to get rid of the following compiler warnings, can you give me hints on how to do that?

extract.c: In function ‘sqfs_stat’:
extract.c:51:17: warning: implicit declaration of function ‘sqfs_makedev’ [-Wimplicit-function-declaration]
   st->st_rdev = sqfs_makedev(inode->xtra.dev.major,
                 ^
extract.c: In function ‘main’:
extract.c:148:91: warning: pointer targets in passing argument 4 of ‘sqfs_read_range’ differ in signedness [-Wpointer-sign]
                         if (sqfs_read_range(&fs, &inode, (sqfs_off_t) bytes_already_read, &bytes_at_a_time, buf))
                                                                                           ^
In file included from squashfuse.h:29:0,
                 from extract.c:1:
file.h:60:10: note: expected ‘sqfs_off_t * {aka long int *}’ but argument is of type ‘size_t * {aka long unsigned int *}’
 sqfs_err sqfs_read_range(sqfs *fs, sqfs_inode *inode, sqfs_off_t start,
          ^
  CCLD     squashfuse_extract
make[1]: Leaving directory '/home/me/squashfuse'

chipturner · 2016-10-10T00:51:07Z

Looks good, thanks!

chipturner · 2016-10-10T00:52:55Z

To fix that warning, you probably need to switch from long int to a size_t (size_t is unsigned)

probonopd added 15 commits October 8, 2016 10:26

Update README

4fd66af

Update README

62a2a22

Update README

2ddb403

Update README

9a5c9c3

Add extract.c

9894c39

Use fwrite

28eeee3

Read the file in chunks

4d7bc31

Extract files and create directories

1e6c506

Handle error on fopen

b049a68

Add symlink functionality

b5a132b

Unlink pre-existing symlink

9ab6ca5

Implement squashfs-root/ prefix

68b8ea6

Fix USAGE statement

ad39378

Implement permissions on the extracted files

891942a

Drop runtime dependency on libfuse by implementing sqfs_stat

e7ea851

chipturner requested changes Oct 9, 2016

View reviewed changes

probonopd added 6 commits October 9, 2016 23:37

Positive exit codes

59ae332

Space before the open paren of if statements

d743c1f

Increase chunk size and use variable buffer size

b4618fc

no need for explicit malloc/free

8276a81

no need for explicit malloc/free

b4708fe

Check return code of sqfs_readlink

9e83e5d

chipturner merged commit 7a7480e into vasi:master Oct 10, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add squashfuse_extract tool #14

Add squashfuse_extract tool #14

probonopd commented Oct 9, 2016 •

edited

Loading

chipturner left a comment

chipturner Oct 9, 2016 •

edited

Loading

probonopd Oct 9, 2016

chipturner Oct 9, 2016 •

edited

Loading

probonopd Oct 9, 2016

chipturner Oct 9, 2016

probonopd Oct 9, 2016

chipturner Oct 9, 2016

probonopd Oct 9, 2016

chipturner Oct 9, 2016

probonopd Oct 9, 2016

chipturner Oct 9, 2016

probonopd Oct 9, 2016

chipturner Oct 9, 2016

probonopd Oct 9, 2016

chipturner Oct 9, 2016

probonopd Oct 9, 2016

chipturner Oct 9, 2016 •

edited

Loading

probonopd Oct 9, 2016

chipturner Oct 9, 2016

probonopd Oct 9, 2016

probonopd commented Oct 9, 2016

chipturner commented Oct 10, 2016

chipturner commented Oct 10, 2016


		prefix = "squashfs-root/";

		if(access(prefix, F_OK ) == -1 ) {

Add squashfuse_extract tool #14

Add squashfuse_extract tool #14

Conversation

probonopd commented Oct 9, 2016 • edited Loading

chipturner left a comment

Choose a reason for hiding this comment

chipturner Oct 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chipturner Oct 9, 2016 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chipturner Oct 9, 2016 • edited Loading

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

probonopd commented Oct 9, 2016

chipturner commented Oct 10, 2016

chipturner commented Oct 10, 2016

probonopd commented Oct 9, 2016 •

edited

Loading

chipturner Oct 9, 2016 •

edited

Loading

chipturner Oct 9, 2016 •

edited

Loading

chipturner Oct 9, 2016 •

edited

Loading