Libguestfs - Apr 2016 - Re: [PATCH v2 3/5] daemon: Added internal_filesystem_walk command

On Monday 04 April 2016 14:58:35 NoxDaFox wrote:
> > > +
> > > +static int open_filesystem (const char *device,
> > > +                            TSK_IMG_INFO **img, TSK_FS_INFO
**fs);
> > > +static TSK_WALK_RET_ENUM fswalk_callback (TSK_FS_FILE *fsfile,
> > > +                                          const char *path, void
*data);
> >
> > Single line for forward declarations.
> >
> 
> Even if they are longer than 80 chars?
Yep.
> > > +static char file_type (TSK_FS_FILE *fsfile);
> > > +static int send_dirent_info (guestfs_int_tsk_dirent *dirent);
> > > +static void reply_with_tsk_error (const char *funcname);
> > > +
> > > +int
> > > +do_internal_filesystem_walk (const mountable_t *mountable)
> > > +{
> > > +  int ret = -1;
> > > +  TSK_FS_INFO *fs = NULL;
> > > +  TSK_IMG_INFO *img = NULL;  /* Used internally by
tsk_fs_dir_walk */
> > > +  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,
fswalk_callback,
> > NULL);
> > > +  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;
> > > +}
> > > +
> > > +/* Inspect the device and initialises the img and fs structures.
> > > + * Return 0 on success, -1 on error.
> > > + */
> > > +static int
> > > +open_filesystem (const char *device, TSK_IMG_INFO **img,
TSK_FS_INFO
> > **fs)
> > > +{
> > > +  const char *images[] = { device };
> > > +
> > > +  *img = tsk_img_open (1, images, TSK_IMG_TYPE_DETECT , 0);
> > > +  if (*img == NULL) {
> > > +    reply_with_tsk_error ("tsk_image_open");
> > > +    return -1;
> > > +  }
> > > +
> > > +  *fs = tsk_fs_open_img (*img, 0, TSK_FS_TYPE_DETECT);
> > > +  if (*fs == NULL) {
> > > +    reply_with_tsk_error ("tsk_fs_open_img");
> > > +    (*img)->close (*img);
> > > +    return -1;
> > > +  }
> > > +
> > > +  return 0;
> > > +}
> > > +
> > > +/* Filesystem walk callback, it gets called on every FS node.
> > > + * Parse the node, encode it into an XDR structure and send it
to the
> > appliance.
> > > + * Return TSK_WALK_CONT on success, TSK_WALK_ERROR on error.
> > > + */
> > > +static TSK_WALK_RET_ENUM
> > > +fswalk_callback (TSK_FS_FILE *fsfile, const char *path, void
*data)
> > > +{
> > > +  int ret = 0;
> > > +  CLEANUP_FREE char *fname = NULL;
> > > +  struct guestfs_int_tsk_dirent dirent;
> > > +
> > > +  /* Ignore ./ and ../ */
> > > +  ret = TSK_FS_ISDOT (fsfile->name->name);
> > > +  if (ret != 0)
> > > +    return TSK_WALK_CONT;
> > > +
> > > +  /* Build the full relative path of the entry */
> > > +  ret = asprintf_nowarn (&fname, "%Q%Q", path,
fsfile->name->name);
> >
> > Why the quoting?  We don't quote results in similar APIs (e.g.
readdir).
> >
> I didn't understand this one. I checked daemon/readdir.c and I found no
> asprintf examples there.
$ ./run guestfish -N fs -m /dev/sda1 touch "/file with spaces" :
readdir /
[0] = {
  ino: 12
  ftyp: r
  name: file with spaces
}
[1] = {
  ino: 2
  ftyp: d
  name: .
}
[2] = {
  ino: 11
  ftyp: d
  name: lost+found
}
[3] = {
  ino: 2
  ftyp: d
  name: ..
}

You can see the file names are not quoted.
> > > +  if (ret < 0) {
> > > +    fprintf (stderr, "asprintf: %m");
> > > +    return TSK_WALK_ERROR;
> > > +  }
> > > +
> > > +  dirent.tsk_inode = fsfile->name->meta_addr;
> > > +  dirent.tsk_type = file_type (fsfile);
> > > +  dirent.tsk_size = (fsfile->meta != NULL) ?
fsfile->meta->size : 0;
> >
> > If 'meta' is null, then I guess the size should be -1 to
indicate it
> > was not available; otherwise, there is no difference between an empty
> > file, and a file whose metadata could not be read.
> >
> 
> The issue is that even if 'meta' is non-null, yet the value could
be 0. In
> cases where the file has been deleted, TSK does its best to retrieve as
> much as it can and set to 0 the rest (same applies with inode for example,
> the inode is set to 0 instead of -1).
> 
> The command documentation reports this "issue" (or feature?).
In this case, the problem is on the library, which reports what can be
a valid file size.  OTOH, if we know for sure that tsk could not
determine the metadata of the file, then let's report that to the
users so they can act depending on that.

Thanks,
-- 
Pino Toscano

On Monday 04 April 2016 23:07:05 noxdafox wrote:
> On 04/04/16 15:15, Pino Toscano wrote:
> > On Monday 04 April 2016 14:58:35 NoxDaFox wrote:
> >>>> +
> >>>> +static int open_filesystem (const char *device,
> >>>> +                            TSK_IMG_INFO **img,
TSK_FS_INFO **fs);
> >>>> +static TSK_WALK_RET_ENUM fswalk_callback (TSK_FS_FILE
*fsfile,
> >>>> +                                          const char
*path, void *data);
> >>> Single line for forward declarations.
> >>>
> >> Even if they are longer than 80 chars?
> > Yep.
> >
> >>>> +static char file_type (TSK_FS_FILE *fsfile);
> >>>> +static int send_dirent_info (guestfs_int_tsk_dirent
*dirent);
> >>>> +static void reply_with_tsk_error (const char *funcname);
> >>>> +
> >>>> +int
> >>>> +do_internal_filesystem_walk (const mountable_t
*mountable)
> >>>> +{
> >>>> +  int ret = -1;
> >>>> +  TSK_FS_INFO *fs = NULL;
> >>>> +  TSK_IMG_INFO *img = NULL;  /* Used internally by
tsk_fs_dir_walk */
> >>>> +  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,
fswalk_callback,
> >>> NULL);
> >>>> +  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;
> >>>> +}
> >>>> +
> >>>> +/* Inspect the device and initialises the img and fs
structures.
> >>>> + * Return 0 on success, -1 on error.
> >>>> + */
> >>>> +static int
> >>>> +open_filesystem (const char *device, TSK_IMG_INFO **img,
TSK_FS_INFO
> >>> **fs)
> >>>> +{
> >>>> +  const char *images[] = { device };
> >>>> +
> >>>> +  *img = tsk_img_open (1, images, TSK_IMG_TYPE_DETECT ,
0);
> >>>> +  if (*img == NULL) {
> >>>> +    reply_with_tsk_error ("tsk_image_open");
> >>>> +    return -1;
> >>>> +  }
> >>>> +
> >>>> +  *fs = tsk_fs_open_img (*img, 0, TSK_FS_TYPE_DETECT);
> >>>> +  if (*fs == NULL) {
> >>>> +    reply_with_tsk_error ("tsk_fs_open_img");
> >>>> +    (*img)->close (*img);
> >>>> +    return -1;
> >>>> +  }
> >>>> +
> >>>> +  return 0;
> >>>> +}
> >>>> +
> >>>> +/* Filesystem walk callback, it gets called on every FS
node.
> >>>> + * Parse the node, encode it into an XDR structure and
send it to the
> >>> appliance.
> >>>> + * Return TSK_WALK_CONT on success, TSK_WALK_ERROR on
error.
> >>>> + */
> >>>> +static TSK_WALK_RET_ENUM
> >>>> +fswalk_callback (TSK_FS_FILE *fsfile, const char *path,
void *data)
> >>>> +{
> >>>> +  int ret = 0;
> >>>> +  CLEANUP_FREE char *fname = NULL;
> >>>> +  struct guestfs_int_tsk_dirent dirent;
> >>>> +
> >>>> +  /* Ignore ./ and ../ */
> >>>> +  ret = TSK_FS_ISDOT (fsfile->name->name);
> >>>> +  if (ret != 0)
> >>>> +    return TSK_WALK_CONT;
> >>>> +
> >>>> +  /* Build the full relative path of the entry */
> >>>> +  ret = asprintf_nowarn (&fname, "%Q%Q",
path, fsfile->name->name);
> >>> Why the quoting?  We don't quote results in similar APIs
(e.g. readdir).
> >>>
> >> I didn't understand this one. I checked daemon/readdir.c and I
found no
> >> asprintf examples there.
> > $ ./run guestfish -N fs -m /dev/sda1 touch "/file with
spaces" : readdir /
> > [0] = {
> >    ino: 12
> >    ftyp: r
> >    name: file with spaces
> > }
> > [1] = {
> >    ino: 2
> >    ftyp: d
> >    name: .
> > }
> > [2] = {
> >    ino: 11
> >    ftyp: d
> >    name: lost+found
> > }
> > [3] = {
> >    ino: 2
> >    ftyp: d
> >    name: ..
> > }
> >
> > You can see the file names are not quoted.
> 
> As long as I'm not missing something, the double quote there is not 
> adding quotes to the names but passing the string formatting parameter 
> to 'asprintf'.
> int asprintf(char **strp, const char *fmt, ...);
> 
> If I remove it, I get this error.
> 
> tsk.c: In function 'fswalk_callback':
> tsk.c:112:34: error: expected expression before '%' token
>     ret = asprintf_nowarn (&fname, %Q%Q, path,
fsfile->name->name);
> 
> If I keep it, I get this output.
> 
> ./run guestfish --ro -a ubuntu.qcow2 run : filesystem_walk /dev/sda1 | less
> [0] = {
>    tsk_inode: 11
>    tsk_type: d
>    tsk_size: 16384
>    tsk_name: lost\+found
>    tsk_allocated: 1
> }
> [1] = {
>    tsk_inode: 12
>    tsk_type: l
>    tsk_size: 33
>    tsk_name: initrd.img
>    tsk_allocated: 1
> }
> 
> As you can see names are not quoted.
Sorry, I said "quoted" because of %Q -- they are escaped.  Just join
path and filename without escaping them.

-- 
Pino Toscano