Autoload of striped images #95

blschatz · 2019-06-07T21:01:53Z

This patch fixes building on current MacOS and adds support for automatically loading dependent striped image containers that are stored in the same folder as a container that has been opened.

Fixes issue Velocidex#92

start of file for "container.description"

CLAassistant · 2019-06-07T21:02:01Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ scudette
✅ blschatz
❌ Bradley

Bradley seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

scudette · 2019-06-12T00:11:08Z

Still waiting on CLA for this. Please also check indentation - seems to be wrong in places.

jtsylve · 2019-06-12T22:52:57Z

I could be reading it wrong, but does this patch auto load any and all aff4 images in the same directory as the one the user is specifying? If so, this patch will break assumptions made by the c bindings. Currently in the C bindings the first AFF4 standard image is opened. If a user has multiple (unrelated) AFF4 images in the same directory, this patch will likely break that assumption in some cases.

I suggest we agree on a naming standard for split/striped images and limit the auto loading to images that fit that standard.

In the past we've discussed image.aff4 as the base image that users would select for loading and image.aff4.%u where %u is a segment number.

Thoughts?

scudette · 2019-06-12T23:31:58Z

This seems to be a convenience patch at best - it just saves a user from adding a * after the file name - i.e. aff4imager -e foobar image.aff4.*.

Do we really care that much? It would not be a good idea if it went off and automatically loaded all the images in the same directory for no reason.

blschatz · 2019-06-13T00:04:54Z

The purpose of the patch is to enable any consumer of the c-aff4 library to open the Striped Canonical Reference Images.

The patch doesn't load all images in the folder. This resolution mechanism is only executed when an open Map contains references to unresolved image Streams, so is not even executed for single container images; for striped images it is very lightweight. What it does is quickly build a map of VolumeID to Container files by scanning the first 1k or so of each AFF4 file in the same folder.

This is the easiest way to ensure interoperability with the Standard. Without the patch, consumers need specify all additional containers required to materialise the Map.

Joe, I agree that creating a standardised naming scheme for striped/multi part images is a good idea moving forward - more from a UX perspective than from a technical one.

blschatz · 2019-06-13T00:20:15Z

Michael,
The spacing related commits make the relevant lines consistent with the wider indent style. The majority of the surrounding code uses spaces, whereas those few lines used tabs.

jtsylve · 2019-06-13T01:17:33Z

This project benefit from the use of a .clang-format file or similar to deal with the formatting issues consistently.

jtsylve · 2019-06-13T01:20:51Z

aff4/aff4_imager_utils.cc

@@ -248,8 +248,7 @@ AFF4Status BasicImager::handle_aff4_volumes() {
                                    AFF4Flusher<AFF4Stream>(backing_stream.release()),
                                    volume));

-                volume_objs.AddVolume(
-                    std::move(AFF4Flusher<AFF4Volume>(volume.release())));
+                volume_objs.AddVolume(std::move(AFF4Flusher<AFF4Volume>(volume.release())));


Remove the std::move from this call, it's not needed and will stop automatic copy elision

scudette · 2019-06-12T23:34:03Z

aff4/aff4_file.h

@@ -46,7 +46,7 @@ class FileBackedObject: public AFF4Stream {
    // The filename for this object.
    std::string filename;

-    explicit FileBackedObject(DataStore* resolver): AFF4Stream(resolver), fd(0){}
+    explicit FileBackedObject(DataStore* resolver, std::string filename): AFF4Stream(resolver), filename(filename), fd(0){}


This change is unnecessary as we have a factory function below. You should never need to create an instance of this type without a unique ptr to manage it.

scudette · 2019-06-13T01:17:56Z

aff4/aff4_utils.h

 #include "spdlog/spdlog.h"

 namespace aff4 {

 #define UNUSED(x) (void)x

-std::string aff4_sprintf(std::string fmt, ...);
+    std::string aff4_sprintf(std::string fmt, ...);


Please indent this and below back. A namespace definition does not increase indentation.

Do you want me to de-indent all of your code too? All the existing code is at a 4 space indent.

https://google.github.io/styleguide/cppguide.html#Namespace_Formatting

The contents of namespaces are not indented.

My mistake. Apologies for the unwarranted whitespace change.

scudette · 2019-06-13T06:08:50Z

aff4/volume_group.cc

-                                        stream_urn, type_str);
-                return STATUS_OK;
-            }
+    AFF4Status VolumeGroup::GetStream(URN stream_urn, AFF4Flusher<AFF4Stream> &result) {


Please indent back

scudette · 2019-06-13T06:09:11Z

aff4/volume_group.cc


 namespace aff4 {

+    void VolumeGroup::AddVolume(AFF4Flusher<AFF4Volume> &&volume) {


Please indent back here and below.

scudette · 2019-06-13T06:12:02Z

aff4/volume_group.cc

+            ScanForAFF4Volumes(path);
+        }
+        // Check known volume URNs.
+        if (foundVolumes.find(urn) != foundVolumes.end()) {


This code looks the same as above - refactor?

scudette · 2019-06-13T06:19:59Z

aff4/aff4_file.cc

@@ -327,9 +327,8 @@ FileBackedObject::~FileBackedObject() {
     std::string mode,
     AFF4Flusher<FileBackedObject> &result
 ) {
-     auto new_object = AFF4Flusher<FileBackedObject>(new FileBackedObject(resolver));
+     auto new_object = AFF4Flusher<FileBackedObject>(new FileBackedObject(resolver, filename));


Just use the factory function

scudette · 2019-06-13T06:25:32Z

aff4/aff4_map.cc

-
+        if((volumes->GetStream(URN(line), target)) != STATUS_OK){
+            // Search in the repository for this target.
+            resolver->logger->info("Attempting to locate resource {} ", line);


I actually do not think this is a good architectural choice - we are breaking down the encapsulation between classes and assuming a particular context. This makes the code more fragile.

I think it is a mistake to lump volume discovery together with the volume group - volume groups are there to manager lifetimes and they currently do not assume a particular volume implementation (like zip files stored in file backed objects). It is the imager which is aware of the details and it should do the right thing.

IMHO the way this should work is - you try to open the map at which point the map object checks that all the named targets are valid. If any are not known it should return an error status to that effect. The imager should then handle the error if it wants and it makes sense by scanning the volumes near the one we started with. Remember that sometimes the volume may not even be stored on a filesystem and it does not make any sense to try to open it here.

scudette · 2019-06-13T06:27:09Z

aff4/libaff4-c.cc

@@ -153,6 +153,7 @@ AFF4_Handle* AFF4_open(const char* filename, AFF4_Message** msg) {
    };

    h->volumes.AddVolume(std::move(zip));
+    h->volumes.AddSearchPath(filename);


search path is not related to the volume group (volumes have no concept of paths). This should be a separate helper object or function.

scudette · 2019-06-13T06:28:42Z

aff4/volume_group.h

+    std::set<std::string> searchPaths;
+    std::unordered_map<URN, std::string> foundVolumes;
+
+    bool FoundVolumesContains(const std::string& filename);


Please separate these into another class and use composition.

scudette · 2019-06-13T06:29:43Z

aff4/zip.cc

@@ -1035,6 +1035,160 @@ AFF4Status ZipFile::StreamAddMember(URN member_urn, AFF4Stream& stream,
    }

    return STATUS_OK;
+    }
+
+    std::string ZipFile::GetResourceID(std::string& filename, std::shared_ptr<spdlog::logger> logger) {


Does this method use the instance at all? Is it just basically a static method?

scudette and others added 11 commits May 25, 2019 15:41

Bug: Zip data descriptor was not written on some code paths.

79d4d3e

Fix bug in map handling

9e057c8

Fixes issue Velocidex#92

Correct and refactor build instructions.

987bcf5

MacOS specific defines and includes.

4c4a4c5

Bugfix for sizeless ImageStream

b971e29

Add fast Volume URN/ARN identification from Zip file by carving the

4d37cdf

start of file for "container.description"

Add multi-container autoload for striped image support

3851234

Merge remote-tracking branch 'Upstream/master'

10eef8f

Merge remote-tracking branch 'Upstream/data_descriptor'

008bf27

Merge remote-tracking branch 'Upstream/scudette-patch-1'

6a66a8b

Fix MacOS install instructions

1dc699f

Merge branch 'master' into master

aaeae31

jtsylve reviewed Jun 13, 2019

View reviewed changes

scudette requested changes Jun 13, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Autoload of striped images #95

Autoload of striped images #95

blschatz commented Jun 7, 2019

CLAassistant commented Jun 7, 2019 •

edited

Loading

scudette commented Jun 12, 2019

jtsylve commented Jun 12, 2019

scudette commented Jun 12, 2019

blschatz commented Jun 13, 2019

blschatz commented Jun 13, 2019

jtsylve commented Jun 13, 2019

jtsylve Jun 13, 2019

scudette Jun 12, 2019

scudette Jun 13, 2019

blschatz Jun 16, 2019

scudette Jun 16, 2019

blschatz Jun 16, 2019

scudette Jun 13, 2019

scudette Jun 13, 2019

scudette Jun 13, 2019

scudette Jun 13, 2019

scudette Jun 13, 2019

scudette Jun 13, 2019

scudette Jun 13, 2019

scudette Jun 13, 2019


		namespace aff4 {

		void VolumeGroup::AddVolume(AFF4Flusher<AFF4Volume> &&volume) {

Autoload of striped images #95

Are you sure you want to change the base?

Autoload of striped images #95

Conversation

blschatz commented Jun 7, 2019

CLAassistant commented Jun 7, 2019 • edited Loading

scudette commented Jun 12, 2019

jtsylve commented Jun 12, 2019

scudette commented Jun 12, 2019

blschatz commented Jun 13, 2019

blschatz commented Jun 13, 2019

jtsylve commented Jun 13, 2019

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

CLAassistant commented Jun 7, 2019 •

edited

Loading