New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

`msLoadMap()` fails to close the mapfile file descriptor after a failed mapfile `INCLUDE` #4871

Closed
wants to merge 3 commits into
base: branch-6-4
from

Conversation

Projects
None yet
3 participants
@tbonfort
Member

tbonfort commented Feb 21, 2014

The following program illustrates the problem. The expectation would be that the mapfile file descriptor is closed when there is an error involving a failed INCLUDE statement. Testing using the mapserver master branch, this is not currently the case.

/**
 * File `test.c`
 *
 * A test program showing that the file descriptor to a mapfile with a non
 * existent INCLUDE value is not closed after `msLoadMap()` returns with an
 * error.  See inline comments for details.
 *
 * Compile along the following lines:
 *
 *    gcc -Wall -I./ -I./build/ -I/usr/include/gdal/ -I/usr/include/libxml2/ ./test.c -o ./test -L/usr/local/lib -lmapserver
 *
 * and run as `./test`.
 */

#include "mapserver.h"

int main(int argc, char *argv[]) {
  /* Open the test mapfile. The following mapfile is a minimal example
   * replicating the test case:

MAP
 NAME "include-mapfile"
 EXTENT 0 0 500 500
 SIZE 250 250

 INCLUDE "non-existent-file.map"
END

  */
  mapObj *map = msLoadMap("./test.map", NULL);

  /* open and close the test mapfile */
  if (!map) {
    msWriteError(stderr);
    msCleanup(0);
  } else {
    msFreeMap(map);             /* shouldn't get here */
    exit(0);
  }

  /* don't exit: we need to be able to look at open file
   * descriptors. e.g. something along the lines of

ls -l /proc/{PID}/fd

   * will show that the mapfile `./test.map` is still open. */
  while (1) {
    sleep(100);
  }

  exit(1); /* shouldn't get here: CTRL-c to exit after looking for the open file. */
}

Fixing this enables geo-data/node-mapserv#13 to be closed.

@tbonfort

This comment has been minimized.

Show comment
Hide comment
@tbonfort

tbonfort Feb 21, 2014

Member

@homme please test the provided patch, I believe it fixes your issue. @sdlime could you check the change I added in maplexer.l for this, I'm not at all certain it's the correct way of fixing?

Member

tbonfort commented Feb 21, 2014

@homme please test the provided patch, I believe it fixes your issue. @sdlime could you check the change I added in maplexer.l for this, I'm not at all certain it's the correct way of fixing?

@tbonfort

This comment has been minimized.

Show comment
Hide comment
@tbonfort

tbonfort Feb 21, 2014

Member

Note that this would also be an issue for fastcgi servers, where such an error could lead up to filling up the maximum number of open files allowed for a process

Member

tbonfort commented Feb 21, 2014

Note that this would also be an issue for fastcgi servers, where such an error could lead up to filling up the maximum number of open files allowed for a process

@homme

This comment has been minimized.

Show comment
Hide comment
@homme

homme Feb 24, 2014

Contributor

@tbonfort Thanks for looking into this. Unfortunately it doesn't seem to have fixed it. I used your issues/4871-fclose-on-include-error branch and compiled the simple test program above against it. Running it from a /data directory results in the following:

root@3c0f5e71aabc:/data# ./test
msLoadMap(): Premature End-of-File.  <br>
msyylex(): Unable to access file. Error opening included file "non-existent-file.map". <br>
^Z
[1]+  Stopped                 ./test
root@3c0f5e71aabc:/data# pgrep -lf test
1600 ./test
root@3c0f5e71aabc:/data# ls -l /proc/1600/fd
total 0
lrwx------ 1 root root 64 Feb 24 10:49 0 -> /dev/pts/2
lrwx------ 1 root root 64 Feb 24 10:49 1 -> /dev/pts/2
lrwx------ 1 root root 64 Feb 24 10:49 2 -> /dev/pts/2
lr-x------ 1 root root 64 Feb 24 10:49 3 -> /data/test.map

I would expect /data/test.map to have been closed...

Contributor

homme commented Feb 24, 2014

@tbonfort Thanks for looking into this. Unfortunately it doesn't seem to have fixed it. I used your issues/4871-fclose-on-include-error branch and compiled the simple test program above against it. Running it from a /data directory results in the following:

root@3c0f5e71aabc:/data# ./test
msLoadMap(): Premature End-of-File.  <br>
msyylex(): Unable to access file. Error opening included file "non-existent-file.map". <br>
^Z
[1]+  Stopped                 ./test
root@3c0f5e71aabc:/data# pgrep -lf test
1600 ./test
root@3c0f5e71aabc:/data# ls -l /proc/1600/fd
total 0
lrwx------ 1 root root 64 Feb 24 10:49 0 -> /dev/pts/2
lrwx------ 1 root root 64 Feb 24 10:49 1 -> /dev/pts/2
lrwx------ 1 root root 64 Feb 24 10:49 2 -> /dev/pts/2
lr-x------ 1 root root 64 Feb 24 10:49 3 -> /data/test.map

I would expect /data/test.map to have been closed...

@tbonfort

This comment has been minimized.

Show comment
Hide comment
@tbonfort

tbonfort Feb 24, 2014

Member

@homme sorry, I committed a version of maplexer.c that had the fix commented out while I was debugging (maplexer.l is correct). I've just updated the pull request:

[tbonfort] ~/dev/mapserver/build (issues/4871-fclose-on-include-error *)
$ ./test 
msLoadMap(): Premature End-of-File.  <br>
msyylex(): Unable to access file. Error opening included file "non-existent-file.map". <br>
^[[A^Z
[2]+  Stopped                 ./test
[tbonfort] ~/dev/mapserver/build (issues/4871-fclose-on-include-error *)
$ bg
[2]+ ./test &
[tbonfort] ~/dev/mapserver/build (issues/4871-fclose-on-include-error *)
$ pgrep -lf test
8579 ./test
[tbonfort] ~/dev/mapserver/build (issues/4871-fclose-on-include-error *)
$ ls -l /proc/8579/fd
total 0
lrwx------ 1 tbonfort tbonfort 64 Feb 24 12:08 0 -> /dev/pts/0
lrwx------ 1 tbonfort tbonfort 64 Feb 24 12:08 1 -> /dev/pts/0
lrwx------ 1 tbonfort tbonfort 64 Feb 24 12:08 2 -> /dev/pts/0
[tbonfort] ~/dev/mapserver/build (issues/4871-fclose-on-include-error *)
Member

tbonfort commented Feb 24, 2014

@homme sorry, I committed a version of maplexer.c that had the fix commented out while I was debugging (maplexer.l is correct). I've just updated the pull request:

[tbonfort] ~/dev/mapserver/build (issues/4871-fclose-on-include-error *)
$ ./test 
msLoadMap(): Premature End-of-File.  <br>
msyylex(): Unable to access file. Error opening included file "non-existent-file.map". <br>
^[[A^Z
[2]+  Stopped                 ./test
[tbonfort] ~/dev/mapserver/build (issues/4871-fclose-on-include-error *)
$ bg
[2]+ ./test &
[tbonfort] ~/dev/mapserver/build (issues/4871-fclose-on-include-error *)
$ pgrep -lf test
8579 ./test
[tbonfort] ~/dev/mapserver/build (issues/4871-fclose-on-include-error *)
$ ls -l /proc/8579/fd
total 0
lrwx------ 1 tbonfort tbonfort 64 Feb 24 12:08 0 -> /dev/pts/0
lrwx------ 1 tbonfort tbonfort 64 Feb 24 12:08 1 -> /dev/pts/0
lrwx------ 1 tbonfort tbonfort 64 Feb 24 12:08 2 -> /dev/pts/0
[tbonfort] ~/dev/mapserver/build (issues/4871-fclose-on-include-error *)
@sdlime

This comment has been minimized.

Show comment
Hide comment
@sdlime

sdlime Feb 24, 2014

@tbonfort, I think your fix is ok... Alternatively we could assign msyyin only after we know it opened successfully:

msyyin_include = fopen(...);
if(!msyyin_include) {
...
}
msyyin = msyyin_include;

A little easier to read this way. --Steve

sdlime commented on 351b059 Feb 24, 2014

@tbonfort, I think your fix is ok... Alternatively we could assign msyyin only after we know it opened successfully:

msyyin_include = fopen(...);
if(!msyyin_include) {
...
}
msyyin = msyyin_include;

A little easier to read this way. --Steve

This comment has been minimized.

Show comment
Hide comment
@tbonfort

tbonfort Feb 28, 2014

Owner

@sdlime: YY_CURRENT_BUFFER contains a reference to msyyin so we could use that directly without using our own temporary variable. However YY_CURRENT_BUFFER is a yacc private variable, so we shouldn't really be peaking into it correct ?

Owner

tbonfort replied Feb 28, 2014

@sdlime: YY_CURRENT_BUFFER contains a reference to msyyin so we could use that directly without using our own temporary variable. However YY_CURRENT_BUFFER is a yacc private variable, so we shouldn't really be peaking into it correct ?

This comment has been minimized.

Show comment
Hide comment
@sdlime

sdlime Mar 7, 2014

Depends what you mean by peeking. Using YY_CURRENT_BUFFER for includes came straight out of Flex documentation/examples.

sdlime replied Mar 7, 2014

Depends what you mean by peeking. Using YY_CURRENT_BUFFER for includes came straight out of Flex documentation/examples.

@homme

This comment has been minimized.

Show comment
Hide comment
@homme

homme Feb 24, 2014

Contributor

@tbonfort the update works fine for me too, thank you.

Contributor

homme commented Feb 24, 2014

@tbonfort the update works fine for me too, thank you.

@tbonfort

This comment has been minimized.

Show comment
Hide comment
@tbonfort

tbonfort Mar 7, 2014

Member

squashed and applied for 6.4.2 in 74f9382
@sdlime OK, I've used YY_CURRENT_BUFFER->yy_input_file instead of a temp variable.
@homme you might want to make sure that the revised version is still OK for you.

Member

tbonfort commented Mar 7, 2014

squashed and applied for 6.4.2 in 74f9382
@sdlime OK, I've used YY_CURRENT_BUFFER->yy_input_file instead of a temp variable.
@homme you might want to make sure that the revised version is still OK for you.

@tbonfort tbonfort closed this Mar 7, 2014

zidge added a commit to mapsherpa/mapserver that referenced this pull request Mar 10, 2014

pagameba added a commit to mapsherpa/mapserver that referenced this pull request May 27, 2014

pagameba added a commit to mapsherpa/mapserver that referenced this pull request Sep 30, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment