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.


-- 
Rob Shearman

[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" <paul.vriens.wine(a)gmail.com> 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 <hurtta+gmane(a)siilo.fmi.fi> writes:
> 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"

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

-- 
Alexandre Julliard
julliard(a)winehq.org

[Paul Vriens]

Alexandre Julliard wrote:
> Kari Hurtta <hurtta+gmane(a)siilo.fmi.fi> writes:
> > 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"
>
> 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.