[rfa] remote-fileio.c, remote_fileio_func_fstat, uninitialized st.st_ino.

[rfa] remote-fileio.c, remote_fileio_func_fstat, uninitialized st.st_ino.

[Michael Snyder]

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

Hi Corinna,

Coverity points out that st.st_ino is un-initialized when it is passed 
to  remote_fileio_to_fio_stat, which uses it.  I have no idea if
initializing it to zero is the right thing to do.  Can you comment?

Thanks,
Michael


[-- Attachment #2: fileio.txt --]
[-- Type: text/plain, Size: 715 bytes --]

2011-03-02  Michael Snyder  <msnyder@vmware.com>

	* remote-fileio.c (remote_fileio_func_fstat): Initialize st_ino
	to zero.

Index: remote-fileio.c
===================================================================
RCS file: /cvs/src/src/gdb/remote-fileio.c,v
retrieving revision 1.40
diff -u -p -u -p -r1.40 remote-fileio.c
--- remote-fileio.c	25 Jan 2011 11:54:00 -0000	1.40
+++ remote-fileio.c	2 Mar 2011 21:41:48 -0000
@@ -1175,6 +1175,7 @@ remote_fileio_func_fstat (char *buf)
       remote_fileio_to_fio_uint (1, fst.fst_dev);
       st.st_mode = S_IFCHR | (fd == FIO_FD_CONSOLE_IN ? S_IRUSR : S_IWUSR);
       st.st_nlink = 1;
+      st.st_ino = 0;
 #ifdef HAVE_GETUID
       st.st_uid = getuid ();
 #else

Re: [rfa] remote-fileio.c, remote_fileio_func_fstat, uninitialized st.st_ino.

[Joel Brobecker]

> 2011-03-02  Michael Snyder  <msnyder@vmware.com>
> 
> 	* remote-fileio.c (remote_fileio_func_fstat): Initialize st_ino
> 	to zero.

FWIW: The initialization is made for the case where the file descriptor
is not a file, so we should be ablet to deduce that the st_ino (inode ID)
is irrelevant and not used on the consumer side... This would suggest
that your change is correct. (you should still wait for Corinna to
comment)

> Index: remote-fileio.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/remote-fileio.c,v
> retrieving revision 1.40
> diff -u -p -u -p -r1.40 remote-fileio.c
> --- remote-fileio.c	25 Jan 2011 11:54:00 -0000	1.40
> +++ remote-fileio.c	2 Mar 2011 21:41:48 -0000
> @@ -1175,6 +1175,7 @@ remote_fileio_func_fstat (char *buf)
>        remote_fileio_to_fio_uint (1, fst.fst_dev);
>        st.st_mode = S_IFCHR | (fd == FIO_FD_CONSOLE_IN ? S_IRUSR : S_IWUSR);
>        st.st_nlink = 1;
> +      st.st_ino = 0;
>  #ifdef HAVE_GETUID
>        st.st_uid = getuid ();
>  #else


-- 
Joel

Re: [rfa] remote-fileio.c, remote_fileio_func_fstat, uninitialized st.st_ino.

[Corinna Vinschen]

On Mar  3 08:36, Joel Brobecker wrote:
> > 2011-03-02  Michael Snyder  <...>
> > 
> > 	* remote-fileio.c (remote_fileio_func_fstat): Initialize st_ino
> > 	to zero.
> 
> FWIW: The initialization is made for the case where the file descriptor
> is not a file, so we should be ablet to deduce that the st_ino (inode ID)
> is irrelevant and not used on the consumer side... This would suggest
> that your change is correct. (you should still wait for Corinna to
> comment)

The descriptors are console descriptors so the inode number doesn't
matter and the patch is ok.

I'm just wondering, can we assume that the stat structure has always an
st_ino member?  THere are checks for st_blocks and st_blksize in place
already.  Is the same required for st_ino?


Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

Re: [rfa] remote-fileio.c, remote_fileio_func_fstat, uninitialized st.st_ino.

[Joel Brobecker]

> I'm just wondering, can we assume that the stat structure has always an
> st_ino member?  THere are checks for st_blocks and st_blksize in place
> already.  Is the same required for st_ino?

I think it's fine, at least for now.  The good news is that this
field is described in the Open Group's page for this type:
http://pubs.opengroup.org/onlinepubs/009695399/basedefs/sys/stat.h.html
So I think we can count on it (although, I don't know enough about C
to know how much authority this has)

But, regardless, I think we can leave it be until such day where
we actually come across a system where the field actually does not
exist... When that happens, we can easily fix the build failure.

-- 
Joel

Re: [rfa] remote-fileio.c, remote_fileio_func_fstat, uninitialized st.st_ino.

[Pedro Alves]

On Thursday 03 March 2011 09:25:32, Joel Brobecker wrote:
> > I'm just wondering, can we assume that the stat structure has always an
> > st_ino member?  THere are checks for st_blocks and st_blksize in place
> > already.  Is the same required for st_ino?
> 
> I think it's fine, at least for now.  The good news is that this
> field is described in the Open Group's page for this type:
> http://pubs.opengroup.org/onlinepubs/009695399/basedefs/sys/stat.h.html
> So I think we can count on it (although, I don't know enough about C
> to know how much authority this has)
> 
> But, regardless, I think we can leave it be until such day where
> we actually come across a system where the field actually does not
> exist... When that happens, we can easily fix the build failure.

Can't we just memset `st' instead?

-- 
Pedro Alves

Re: [rfa] remote-fileio.c, remote_fileio_func_fstat, uninitialized st.st_ino.

[Corinna Vinschen]

On Mar  3 10:04, Pedro Alves wrote:
> On Thursday 03 March 2011 09:25:32, Joel Brobecker wrote:
> > > I'm just wondering, can we assume that the stat structure has always an
> > > st_ino member?  THere are checks for st_blocks and st_blksize in place
> > > already.  Is the same required for st_ino?
> > 
> > I think it's fine, at least for now.  The good news is that this
> > field is described in the Open Group's page for this type:
> > http://pubs.opengroup.org/onlinepubs/009695399/basedefs/sys/stat.h.html
> > So I think we can count on it (although, I don't know enough about C
> > to know how much authority this has)
> > 
> > But, regardless, I think we can leave it be until such day where
> > we actually come across a system where the field actually does not
> > exist... When that happens, we can easily fix the build failure.
> 
> Can't we just memset `st' instead?

That would be the simplest solution.  Afterwards, just set the
non-zero fields.


Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

Re: [rfa] remote-fileio.c, remote_fileio_func_fstat, uninitialized st.st_ino.

[Michael Snyder]

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

Corinna Vinschen wrote:
> On Mar  3 10:04, Pedro Alves wrote:
>> On Thursday 03 March 2011 09:25:32, Joel Brobecker wrote:
>>>> I'm just wondering, can we assume that the stat structure has always an
>>>> st_ino member?  THere are checks for st_blocks and st_blksize in place
>>>> already.  Is the same required for st_ino?
>>> I think it's fine, at least for now.  The good news is that this
>>> field is described in the Open Group's page for this type:
>>> http://pubs.opengroup.org/onlinepubs/009695399/basedefs/sys/stat.h.html
>>> So I think we can count on it (although, I don't know enough about C
>>> to know how much authority this has)
>>>
>>> But, regardless, I think we can leave it be until such day where
>>> we actually come across a system where the field actually does not
>>> exist... When that happens, we can easily fix the build failure.
>> Can't we just memset `st' instead?
> 
> That would be the simplest solution.  Afterwards, just set the
> non-zero fields.

OK, how about this?


[-- Attachment #2: fileio2.txt --]
[-- Type: text/plain, Size: 1053 bytes --]

2011-03-03  Michael Snyder  <msnyder@vmware.com>

	* remote-fileio.c (remote_fileio_func_fstat): Initialize all
	fields of struct 'st' to zero.

Index: remote-fileio.c
===================================================================
RCS file: /cvs/src/src/gdb/remote-fileio.c,v
retrieving revision 1.40
diff -u -p -u -p -r1.40 remote-fileio.c
--- remote-fileio.c	25 Jan 2011 11:54:00 -0000	1.40
+++ remote-fileio.c	3 Mar 2011 17:20:06 -0000
@@ -1173,20 +1173,15 @@ remote_fileio_func_fstat (char *buf)
   if (fd == FIO_FD_CONSOLE_IN || fd == FIO_FD_CONSOLE_OUT)
     {
       remote_fileio_to_fio_uint (1, fst.fst_dev);
+      memset (&st, 0, sizeof (st));
       st.st_mode = S_IFCHR | (fd == FIO_FD_CONSOLE_IN ? S_IRUSR : S_IWUSR);
       st.st_nlink = 1;
 #ifdef HAVE_GETUID
       st.st_uid = getuid ();
-#else
-      st.st_uid = 0;
 #endif
 #ifdef HAVE_GETGID
       st.st_gid = getgid ();
-#else
-      st.st_gid = 0;
 #endif
-      st.st_rdev = 0;
-      st.st_size = 0;
 #ifdef HAVE_STRUCT_STAT_ST_BLKSIZE
       st.st_blksize = 512;
 #endif

Re: [rfa] remote-fileio.c, remote_fileio_func_fstat, uninitialized st.st_ino.

[Corinna Vinschen]

On Mar  3 09:21, Michael Snyder wrote:
> Corinna Vinschen wrote:
> >On Mar  3 10:04, Pedro Alves wrote:
> >>Can't we just memset `st' instead?
> >
> >That would be the simplest solution.  Afterwards, just set the
> >non-zero fields.
> 
> OK, how about this?
> 

> 2011-03-03  Michael Snyder  <...>
> 
> 	* remote-fileio.c (remote_fileio_func_fstat): Initialize all
> 	fields of struct 'st' to zero.
> 
> Index: remote-fileio.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/remote-fileio.c,v
> retrieving revision 1.40
> diff -u -p -u -p -r1.40 remote-fileio.c
> --- remote-fileio.c	25 Jan 2011 11:54:00 -0000	1.40
> +++ remote-fileio.c	3 Mar 2011 17:20:06 -0000
> @@ -1173,20 +1173,15 @@ remote_fileio_func_fstat (char *buf)
>    if (fd == FIO_FD_CONSOLE_IN || fd == FIO_FD_CONSOLE_OUT)
>      {
>        remote_fileio_to_fio_uint (1, fst.fst_dev);
> +      memset (&st, 0, sizeof (st));
>        st.st_mode = S_IFCHR | (fd == FIO_FD_CONSOLE_IN ? S_IRUSR : S_IWUSR);
>        st.st_nlink = 1;
>  #ifdef HAVE_GETUID
>        st.st_uid = getuid ();
> -#else
> -      st.st_uid = 0;
>  #endif
>  #ifdef HAVE_GETGID
>        st.st_gid = getgid ();
> -#else
> -      st.st_gid = 0;
>  #endif
> -      st.st_rdev = 0;
> -      st.st_size = 0;
>  #ifdef HAVE_STRUCT_STAT_ST_BLKSIZE
>        st.st_blksize = 512;
>  #endif

Perfect.

Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

Re: [rfa] remote-fileio.c, remote_fileio_func_fstat, uninitialized st.st_ino.

[Michael Snyder]

Corinna Vinschen wrote:
> On Mar  3 09:21, Michael Snyder wrote:
>> Corinna Vinschen wrote:
>>> On Mar  3 10:04, Pedro Alves wrote:
>>>> Can't we just memset `st' instead?
>>> That would be the simplest solution.  Afterwards, just set the
>>> non-zero fields.
>> OK, how about this?
>>
> 
>> 2011-03-03  Michael Snyder  <...>
>>
>> 	* remote-fileio.c (remote_fileio_func_fstat): Initialize all
>> 	fields of struct 'st' to zero.
>>
>> Index: remote-fileio.c
>> ===================================================================
>> RCS file: /cvs/src/src/gdb/remote-fileio.c,v
>> retrieving revision 1.40
>> diff -u -p -u -p -r1.40 remote-fileio.c
>> --- remote-fileio.c	25 Jan 2011 11:54:00 -0000	1.40
>> +++ remote-fileio.c	3 Mar 2011 17:20:06 -0000
>> @@ -1173,20 +1173,15 @@ remote_fileio_func_fstat (char *buf)
>>    if (fd == FIO_FD_CONSOLE_IN || fd == FIO_FD_CONSOLE_OUT)
>>      {
>>        remote_fileio_to_fio_uint (1, fst.fst_dev);
>> +      memset (&st, 0, sizeof (st));
>>        st.st_mode = S_IFCHR | (fd == FIO_FD_CONSOLE_IN ? S_IRUSR : S_IWUSR);
>>        st.st_nlink = 1;
>>  #ifdef HAVE_GETUID
>>        st.st_uid = getuid ();
>> -#else
>> -      st.st_uid = 0;
>>  #endif
>>  #ifdef HAVE_GETGID
>>        st.st_gid = getgid ();
>> -#else
>> -      st.st_gid = 0;
>>  #endif
>> -      st.st_rdev = 0;
>> -      st.st_size = 0;
>>  #ifdef HAVE_STRUCT_STAT_ST_BLKSIZE
>>        st.st_blksize = 512;
>>  #endif
> 
> Perfect.
> 
> Thanks,
> Corinna
> 

Committed.
Thanks,
Michael