GRUB 1.95 cannot read the ufs filesystem

GRUB 1.95 cannot read the ufs filesystem

[Hitoshi Ozeki]

[-- Attachment #1: Type: text/plain, Size: 244 bytes --]

Hello, all.

GRUB 1.95 cannot read the ufs filesystem.
Because, 1.95 mistakes a usage of char pointer.
This patch fixes this problem, and make it a bit better.

Attached file: ufs.patch.gz (1Kbytes)

-- 
 Hitoshi Ozeki h-ozeki@ck2.so-net.ne.jp

[-- Attachment #2: ufs.patch.gz --]
[-- Type: application/x-gzip-compressed, Size: 1085 bytes --]

Re: GRUB 1.95 cannot read the ufs filesystem

[Johan Rydberg]

[-- Attachment #1: Type: text/plain, Size: 489 bytes --]

"Hitoshi Ozeki" <h-ozeki@ck2.so-net.ne.jp> writes:

> GRUB 1.95 cannot read the ufs filesystem.
> Because, 1.95 mistakes a usage of char pointer.
> This patch fixes this problem, and make it a bit better.

Thanks for you contribution.

> Attached file: ufs.patch.gz (1Kbytes)

In the future, could you maybe attach patches uncompressed, and
inlined (or with mime-type text/plain or similar) so we can review the
patch directly within the mail, and give you comments.

Thanks again,
Johan.

[-- Attachment #2: Type: application/pgp-signature, Size: 190 bytes --]

Re: GRUB 1.95 cannot read the ufs filesystem

[Johan Rydberg]

[-- Attachment #1: Type: text/plain, Size: 341 bytes --]

"Hitoshi Ozeki" <h-ozeki@ck2.so-net.ne.jp> writes:

> Hello, all.
>
> GRUB 1.95 cannot read the ufs filesystem.
> Because, 1.95 mistakes a usage of char pointer.
> This patch fixes this problem, and make it a bit better.

Could you maybe point out the problem for us?  Personally, I think the
use of pointers make the code more complex.

~j

[-- Attachment #2: Type: application/pgp-signature, Size: 190 bytes --]

RE: GRUB 1.95 cannot read the ufs filesystem

[Hitoshi Ozeki]

Hello, all.

	Johan Rydberg wrote:
	> GRUB 1.95 cannot read the ufs filesystem.
	> This patch fixes this problem, and make it a bit better.
	Could you maybe point out the problem for us?  
	Personally, I think the use of pointers make the code more complex.

At first, When we use the 'strcmp' for the purpose of comparision C-strings,
It requires to terminate with the NIL sentry('\0').

----begin-------------------------------
grub_ufs_find_file (struct grub_ufs_data *data, const char *path)
{
  char fpath[grub_strlen (path)];  <-- not enough.
  char *name = fpath;
  char *next;
  unsigned int pos = 0;
  int dirino;
  
  grub_strncpy (fpath, path, grub_strlen (path));   <--without NIL.
----end---------------------------------

From the manpage of 'strncpy(3)':
       The  strcpy()  function  copies the string pointed to by src
(including
       the terminating `\0' character) to the array pointed to by  dest.
The
       strings  may not overlap, and the destination string dest must be
large
       enough to receive the copy.

       The strncpy() function is similar, except that not more than n bytes
of
       src  are copied. Thus, if there is no null byte among the first n
bytes
       of src, the result will not be null-terminated.

At second, However we have the filepath in 'const char* path',
The original code copies the filepath. It wastes the processor time.

If you think the use of pointers makes complex,
I recommend to use array and index.

In addition, ufs filesystem has no label.

----begin-------------------------------
static struct grub_fs grub_ufs_fs =
  {
    .name = "ufs",
    .dir = grub_ufs_dir,
    .open = grub_ufs_open,
    .read = grub_ufs_read,
    .close = grub_ufs_close,
    .label = grub_ufs_label,
    .next = 0
  };
----end---------------------------------

The '.label' should set to 0.
On the original code, The 'label' function returns the invalid pointer,
so the 'ls -l' command gets wrong.

-- 
Regard,
 Hitoshi Ozeki

Re: GRUB 1.95 cannot read the ufs filesystem

[Johan Rydberg]

[-- Attachment #1: Type: text/plain, Size: 834 bytes --]

"Hitoshi Ozeki" <h-ozeki@ck2.so-net.ne.jp> writes:

> At first, When we use the 'strcmp' for the purpose of comparision C-strings,
> It requires to terminate with the NIL sentry('\0').
>
> ----begin-------------------------------
> grub_ufs_find_file (struct grub_ufs_data *data, const char *path)
> {
>   char fpath[grub_strlen (path)];  <-- not enough.
>   char *name = fpath;
>   char *next;
>   unsigned int pos = 0;
>   int dirino;
>   
>   grub_strncpy (fpath, path, grub_strlen (path));   <--without NIL.
> ----end---------------------------------

It feels easier to just add +1 at both the location.  Can anyone else
comment on this, please?

> The '.label' should set to 0.
> On the original code, The 'label' function returns the invalid pointer,
> so the 'ls -l' command gets wrong.

Marco?

~j

[-- Attachment #2: Type: application/pgp-signature, Size: 190 bytes --]

Re: GRUB 1.95 cannot read the ufs filesystem

[Johan Rydberg]

[-- Attachment #1: Type: text/plain, Size: 834 bytes --]

"Hitoshi Ozeki" <h-ozeki@ck2.so-net.ne.jp> writes:

> At first, When we use the 'strcmp' for the purpose of comparision C-strings,
> It requires to terminate with the NIL sentry('\0').
>
> ----begin-------------------------------
> grub_ufs_find_file (struct grub_ufs_data *data, const char *path)
> {
>   char fpath[grub_strlen (path)];  <-- not enough.
>   char *name = fpath;
>   char *next;
>   unsigned int pos = 0;
>   int dirino;
>   
>   grub_strncpy (fpath, path, grub_strlen (path));   <--without NIL.
> ----end---------------------------------

It feels easier to just add +1 at both the location.  Can anyone else
comment on this, please?

> The '.label' should set to 0.
> On the original code, The 'label' function returns the invalid pointer,
> so the 'ls -l' command gets wrong.

Marco?

~j

[-- Attachment #2: Type: application/pgp-signature, Size: 190 bytes --]

RE: GRUB 1.95 cannot read the ufs filesystem

[Hitoshi Ozeki]

Hello,all.

I wrote in the last:
	At second, However we have the filepath in 'const char* 
	path', The original code copies the filepath. It wastes 
	the processor time.
	If you think the use of pointers makes complex,
	I recommend to use array and index.

It doesn't only waste the processor time.

The original code uses a large amount of  auto-variable. It wastes the stack
space.
When we exhaust the stack space,  we should expect the worst result.

GRUB is a bootloader, It does not support the virtual memory.

So we must stingy about the auto-variables.

-- 
Regards,
 Hitoshi Ozeki

Re: GRUB 1.95 cannot read the ufs filesystem

[Yoshinori K. Okuji]

On Tuesday 17 April 2007 20:31, Hitoshi Ozeki wrote:
> In addition, ufs filesystem has no label.
>
> ----begin-------------------------------
> static struct grub_fs grub_ufs_fs =
>   {
>     .name = "ufs",
>     .dir = grub_ufs_dir,
>     .open = grub_ufs_open,
>     .read = grub_ufs_read,
>     .close = grub_ufs_close,
>     .label = grub_ufs_label,
>     .next = 0
>   };
> ----end---------------------------------
>
> The '.label' should set to 0.
> On the original code, The 'label' function returns the invalid pointer,
> so the 'ls -l' command gets wrong.

That is right, but I think UFS2 supports a volume name. Does anybody know it?

Thanks,
Okuji

Re: GRUB 1.95 cannot read the ufs filesystem

[Bean]

i think volume name of ufs2 is stored at offset 680–711 in the super block.

btw, i also find out that volume name of fat is incorrect if
international character is used.

On 9/4/07, Yoshinori K. Okuji <okuji@enbug.org> wrote:
> On Tuesday 17 April 2007 20:31, Hitoshi Ozeki wrote:
> > In addition, ufs filesystem has no label.
> >
> > ----begin-------------------------------
> > static struct grub_fs grub_ufs_fs =
> >   {
> >     .name = "ufs",
> >     .dir = grub_ufs_dir,
> >     .open = grub_ufs_open,
> >     .read = grub_ufs_read,
> >     .close = grub_ufs_close,
> >     .label = grub_ufs_label,
> >     .next = 0
> >   };
> > ----end---------------------------------
> >
> > The '.label' should set to 0.
> > On the original code, The 'label' function returns the invalid pointer,
> > so the 'ls -l' command gets wrong.
>
> That is right, but I think UFS2 supports a volume name. Does anybody know it?
>
> Thanks,
> Okuji
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>


-- 
Bean

RE: GRUB 1.95 cannot read the ufs filesystem

[Hitoshi Ozeki]

Hello.

On 17 April 2007, I wrote:
	--- begin grub-1.95/fs/ufs.c ----------------------------------
	static struct grub_fs grub_ufs_fs =
	  {
	    .name = "ufs",
	    .dir = grub_ufs_dir,
	    .open = grub_ufs_open,
	    .read = grub_ufs_read,
	    .close = grub_ufs_close,
	    .label = grub_ufs_label,
	    .next = 0
	  };
	--- end grub-1.95/fs/ufs.c ------------------------------------
	The '.label' should set to 0.

On 4 September 2007, Okuji wrote:
	but I think UFS2 supports a volume name. 
	
This source is function "grub_normal_print_device_info()"
of the "grub-1.95/normal/misc.c".

--- begin grub-1.95/normal/misc.c -----------------------------
  dev = grub_device_open (name);
  if (! dev)
    grub_printf ("Filesystem cannot be accessed");
  else if (! dev->disk || ! dev->disk->has_partitions ||
dev->disk->partition)
    {
      char *label;
      grub_fs_t fs;

      fs = grub_fs_probe (dev);
      /* Ignore all errors.  */
      grub_errno = 0;

      grub_printf ("Filesystem type %s", fs ? fs->name : "unknown");
	  
      if (fs && fs->label) 
	{
	  (fs->label) (dev, &label);		<----  We expect that C
string will be set as a 'label.'
	  if (grub_errno == GRUB_ERR_NONE)
	    {
	      if (label && grub_strlen (label)) 	<--- ouch! 'label'
is not C string.
		grub_printf (", Label %s", label);
	      grub_free (label);			<--- gaaaaah!
	    }
	  grub_errno = GRUB_ERR_NONE;
	}
--- end grub-1.95/normal/misc.c -------------------------------

In ufs.c,  "label" function does nothing.
but, In "normal/misc.c" expects that "ufs.c" allocates a memory and
terminates with NIL.

Even if we did not have any return value in the "label" function, it is not
good to do nothing.

static grub_err_t
grub_ufs_label (grub_device_t device __attribute ((unused)),
		char **label)
{
  *label = grub_malloc (1);
  **label = '\0';
  return GRUB_ERR_NONE;
}

-- 
Regards,
 Hitoshi Ozeki