Question some code in server/request.c

[Paul Vriens]

Hi,

while going through the Coverity reports I found CID-293 that mentions
a possible NULL-RETURN. Marcus sent in a patch that wasn't applied.

If I look at the code (starting at line 537):

/* create the server directory and chdir to it */
static void create_server_dir( const char *dir )
{
    char *p, *server_dir;
    struct stat st, st2;

if (!(server_dir = strdup( dir ))) fatal_error( "out of memory\n" );

/* first create the base directory if needed */

p = strrchr( server_dir, '/' );
    *p = 0;
    create_dir( server_dir, &st );

/* now create the server directory */

*p = '/';
    create_dir( server_dir, &st );

if (chdir( server_dir ) == -1) fatal_perror( "chdir %s", server_dir );
    if (stat( ".", &st2 ) == -1) fatal_perror( "stat %s", server_dir );
    if (st.st_dev != st2.st_dev || st.st_ino != st2.st_ino)
        fatal_error( "chdir did not end up in %s\n", server_dir );

free( server_dir );
}

it looks to me that 'p' is not used at all (this is so since June
2002). Am I completely missing something?

Cheers,

Paul.

[Robert Shearman]

Paul Vriens wrote:
> Hi,
>
> while going through the Coverity reports I found CID-293 that mentions
> a possible NULL-RETURN. Marcus sent in a patch that wasn't applied.
>
> If I look at the code (starting at line 537):
>
> /* create the server directory and chdir to it */
> static void create_server_dir( const char *dir )
> {
>    char *p, *server_dir;
>    struct stat st, st2;
>
> if (!(server_dir = strdup( dir ))) fatal_error( "out of memory\n" );
>
> /* first create the base directory if needed */
>
> p = strrchr( server_dir, '/' );
>    *p = 0;
>    create_dir( server_dir, &st );
>
> /* now create the server directory */
>
> *p = '/';
>    create_dir( server_dir, &st );
>
> if (chdir( server_dir ) == -1) fatal_perror( "chdir %s", server_dir );
>    if (stat( ".", &st2 ) == -1) fatal_perror( "stat %s", server_dir );
>    if (st.st_dev != st2.st_dev || st.st_ino != st2.st_ino)
>        fatal_error( "chdir did not end up in %s\n", server_dir );
>
> free( server_dir );
> }
>
> it looks to me that 'p' is not used at all (this is so since June
> 2002). Am I completely missing something?

p is an alias of server_dir.

[Marcus Meissner]

On Tue, Mar 06, 2007 at 01:34:45PM +0000, Robert Shearman wrote:
> Paul Vriens wrote:
> > Hi,
> >
> > while going through the Coverity reports I found CID-293 that mentions
> > a possible NULL-RETURN. Marcus sent in a patch that wasn't applied.
> >
> > If I look at the code (starting at line 537):
> >
> > /* create the server directory and chdir to it */
> > static void create_server_dir( const char *dir )
> > {
> >   char *p, *server_dir;
> >   struct stat st, st2;
> >
> > if (!(server_dir = strdup( dir ))) fatal_error( "out of memory\n" );
> >
> > /* first create the base directory if needed */
> >
> > p = strrchr( server_dir, '/' );
> >   *p = 0;
> >   create_dir( server_dir, &st );
> >
> > /* now create the server directory */
> >
> > *p = '/';
> >   create_dir( server_dir, &st );
> >
> > if (chdir( server_dir ) == -1) fatal_perror( "chdir %s", server_dir );
> >   if (stat( ".", &st2 ) == -1) fatal_perror( "stat %s", server_dir );
> >   if (st.st_dev != st2.st_dev || st.st_ino != st2.st_ino)
> >       fatal_error( "chdir did not end up in %s\n", server_dir );
> >
> > free( server_dir );
> > }
> >
> > it looks to me that 'p' is not used at all (this is so since June
> > 2002). Am I completely missing something?
>
> p is an alias of server_dir.

aka "it points into the memory of server_dir" ... see the *p parts.

I guess it was not applied since the path always contains a '/'.

Ciao, Marcus

[Kari Hurtta]

"Paul Vriens"  writes
in gmane.comp.emulators.wine.devel:
> Hi,
>
> while going through the Coverity reports I found CID-293 that mentions
> a possible NULL-RETURN. Marcus sent in a patch that wasn't applied.
>
> If I look at the code (starting at line 537):
>
> /* create the server directory and chdir to it */
> static void create_server_dir( const char *dir )
> {
>     char *p, *server_dir;
>     struct stat st, st2;
> if (!(server_dir = strdup( dir ))) fatal_error( "out of memory\n" );
>
> /* first create the base directory if needed */
>
> p = strrchr( server_dir, '/' );
> *p = 0;

=======
> create_dir( server_dir, &st );
>
> /* now create the server directory */
>
> *p = '/';

========
> create_dir( server_dir, &st );
>
> if (chdir( server_dir ) == -1) fatal_perror( "chdir %s", server_dir );
> if (stat( ".", &st2 ) == -1) fatal_perror( "stat %s", server_dir );
> if (st.st_dev != st2.st_dev || st.st_ino != st2.st_ino)
>     fatal_error( "chdir did not end up in %s\n", server_dir );
>
> free( server_dir );
>
> }
>
> it looks to me that 'p' is not used at all (this is so since June
> 2002). Am I completely missing something?

'p' is used on underlined places.

Code assumes that 'dir' is for "/dir1/dir2"

1) Code crashes if there is no '/' on 'dir' at all.

and

2) Code does wrong thing if dir is just "/dir2"

Perhaps it should be:

/* first create the base directory if needed */

p = strrchr( server_dir, '/' );
     if (p && p > server_dir) {
        *p = 0;

create_dir( server_dir, &st );

/* now create the server directory */

*p = '/';
     }

create_dir( server_dir, &st );
> Cheers,
>
> Paul.

/ Kari Hurtta

[Alexandre Julliard]

Kari Hurtta  writes:
> Code assumes that 'dir' is for "/dir1/dir2"
> - Code crashes if there is no '/' on 'dir' at all.
>
> and
> - Code does wrong thing if dir is just "/dir2"

Neither can happen. The server dir is in format /tmp/.wine-%u/server-%x-%x.

[Paul Vriens]

Alexandre Julliard wrote:
> Kari Hurtta  writes:
> > Code assumes that 'dir' is for "/dir1/dir2"
> > - Code crashes if there is no '/' on 'dir' at all.
> >
> > and
> > - Code does wrong thing if dir is just "/dir2"
>
> Neither can happen. The server dir is in format /tmp/.wine-%u/server-%x-%x.

Yeah,

I already figured out my misreading after Robert has sent the email.

I'll mark this one as FALSE on the Coverity website.

Cheers,

Paul.