2016-09-20 11:38 GMT+03:00 Pino Toscano :
> On Monday, 19 September 2016 23:26:57 CEST Matteo Cafasso wrote:
> > The internal_find_block command searches all entries referring to the
> > given filesystem data block and returns a tsk_dirent structure
> > for each of them.
> >
> > For filesystems such as NTFS which do not delete the block mapping
> > when removing files, it is possible to get multiple non-allocated
> > entries for the same block.
> >
> > The gathered list of tsk_dirent structs is serialised into XDR format
> > and written to a file by the appliance.
> >
> > Signed-off-by: Matteo Cafasso
> > ---
>
> The idea is fine, there are few notes below.
>
> > +int
> > +do_internal_find_block (const mountable_t *mountable, int64_t block)
> > +{
> > + int ret = -1;
> > + TSK_FS_INFO *fs = NULL;
> > + TSK_IMG_INFO *img = NULL; /* Used internally by tsk_fs_dir_walk */
> > + const int flags =
> > +TSK_FS_DIR_WALK_FLAG_ALLOC | TSK_FS_DIR_WALK_FLAG_UNALLOC |
> > +TSK_FS_DIR_WALK_FLAG_RECURSE | TSK_FS_DIR_WALK_FLAG_NOORPHAN;
> > +
> > + ret = open_filesystem (mountable->device, &img, &fs);
> > + if (ret < 0)
> > +return ret;
> > +
> > + reply (NULL, NULL); /* Reply message. */
> > +
> > + ret = tsk_fs_dir_walk (fs, fs->root_inum, flags,
> > + findblk_callback, (void *) &block);
> > + if (ret == 0)
> > +ret = send_file_end (0); /* File transfer end. */
> > + else
> > +send_file_end (1); /* Cancel file transfer. */
> > +
> > + fs->close (fs);
> > + img->close (img);
> > +
> > + return ret;
> > +}
> > +
> > +
>
> (One extra empty line.)
>
> > +static TSK_WALK_RET_ENUM
> > +findblk_callback (TSK_FS_FILE *fsfile, const char *path, void *data)
> > +{
> > + findblk_data blkdata;
> > + const TSK_FS_ATTR *fsattr = NULL;
> > + int ret = 0, count = 0, index = 0;
> > + uint64_t *block = (uint64_t *) data;
>
> Just dereference the pointer directly, so it will not be needed later
> on:
>
> const uint64_t block = * (uint64_t *) data;
>
> Or, even better, this can be done only when needed, ...
>
> > + const int flags = TSK_FS_FILE_WALK_FLAG_AONLY |
> TSK_FS_FILE_WALK_FLAG_SLACK;
> > +
> > + if (entry_is_dot (fsfile))
> > +return TSK_WALK_CONT;
> > +
> > + blkdata.found = false;
> > + blkdata.block = *block;
>
> ... here:
>
> blkdata.block = * (uint64_t *) data;
>
> > + /* Retrieve block list */
> > + count = tsk_fs_file_attr_getsize (fsfile);
> > +
> > + for (index = 0; index < count; index++) {
> > +fsattr = tsk_fs_file_attr_get_idx (fsfile, index);
> > +
> > +if (fsattr != NULL && fsattr->flags & TSK_FS_ATTR_NONRES)
> > + tsk_fs_attr_walk (fsattr, flags, attrwalk_callback, (void*)
> &blkdata);
>
> (Minor style nitpick: space missing between "void" and "*".)
>
> Should the return value of tsk_fs_attr_walk be checked for failures,
> and return TSK_WALK_ERROR in case of error?
>
> > +/* Attribute walk, searches the given block within the FS node
> attributes. */
>
> Even if within 80 columns, I'd split it at 70 for readability.
>
> > +static TSK_WALK_RET_ENUM
> > +attrwalk_callback (TSK_FS_FILE *fsfile, TSK_OFF_T offset,
> > + TSK_DADDR_T blkaddr, char *buf, size_t size,
> > + TSK_FS_BLOCK_FLAG_ENUM flags, void *data)
> > +{
> > + findblk_data *blkdata = (findblk_data *) data;
> > +
> > + if (blkaddr == blkdata->block) {
> > +blkdata->found = true;
>
> If I read the sleuthkit API docs, blkaddr will be meaningful only if
> flags contains TSK_FS_BLOCK_FLAG_RAW. Should attrwalk_callback check
> for it?
>
I was thinking the same but I then took a look at its usage within the
sleuthkit tool and it seems not being checked.
https://github.com/sleuthkit/sleuthkit/blob/develop/tsk/fs/ifind_lib.c#L479
Not sure whether to trust the API docs or the code. My main concern is
ignoring some relevant blocks.
I will test it with both solutions and see.
I will also fix the rest.
Thanks for the comments.
>
> Thanks,
> --
> Pino Toscano
> ___
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
>
___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs
