[poppler] [PATCH] Fix GDir's handling of return values from FindFirstFile

[Adam Batkin]

> Can you please provide a better way to see this problem?
> Where did you got the binary from (KDE binary package?)?
> So far the patch looks ok, the POPPLER_DATADIR is a more or less serious 
> bug since we cannot really assume a fixed directory for the data.

I believe that I got the binary from the kde-windows project, though I'm 
not sure that is relevant here. If you work backwards through the code, 
you can *see* the problem.

Start in poppler/GlobalParams.cc, GlobalParams::scanEncodingDirs():
  655   GDir *dir;
  656   GDirEntry *entry;
  657
  658   dir = new GDir(POPPLER_DATADIR "/nameToUnicode", gTrue);
  659   while (entry = dir->getNextEntry(), entry != NULL) {
  660     if (!entry->isDir()) {
  661       parseNameToUnicode(entry->getFullPath());
  662     }
  663     delete entry;
  664   }

So we see that a GDir is constructed on line 658, and on my system the 
directory that it "points" to doesn't exist.

So move on to the GDir constructor in goo/gfile.cc and skip down a few 
lines:

  645   hnd = FindFirstFile(tmp->getCString(), &ffd);

hnd is never touched again in the constructor, nor is ffd which was 
*supposed* to be populated by FindFirstFile(). At this point, hnd is 
either a valid handle, or else it is INVALID_HANDLE_VALUE. In my case, 
it's INVALID_HANDLE_VALUE (-1).

Jumping back to GlobalParams::scanEncodingDirs(), we see that 
dir->getNextEntry() is called, so let's take a look at that:
  675 #if defined(WIN32)
  676   if (hnd) {
  677     e = new GDirEntry(path->getCString(), ffd.cFileName, doStat);
  678     if (hnd  && !FindNextFile(hnd, &ffd)) {
  679       FindClose(hnd);
  680       hnd = NULL;
  681     }
  682   } else {
  683     e = NULL;
  684   }

On line 676 hnd==-1 so the if evalutes to true and a GDirEntry is 
constructed. The GDirEntry is the concatenation of path->getCString() 
(which is valid) and ffd.cFileName (in theory populated above in the 
call to FindFirstFile() in GDir::GDir() line 645 above) but ffd may 
point to garbage (in fact it does) since it was never properly populated!

Taking a quick look at GDirEntry's constructor:
  600 GDirEntry::GDirEntry(char *dirPath, char *nameA, GBool doStat) {
  601 #ifdef VMS
  602   char *p;
  603 #elif defined(WIN32)
  604   int fa;
  605 #elif defined(ACORN)
  606 #else
  607   struct stat st;
  608 #endif
  609
  610   name = new GooString(nameA);
  611   dir = gFalse;
  612   fullPath = new GooString(dirPath);
  613   appendToPath(fullPath, nameA);
We see that fullPath and nameA are concatenated together (nameA is the 
invalid ffd.cFileName from above).

Back in GlobalParams::scanEncodingDirs(), Line 660 evaluates to true 
since "dir" (entry->isDir() is false) since it's not pointing to a 
directory and parseNameToUnicode(entry->getFullPath()) is called which 
is where we get that funky string from (entry->getFullPath() returns the 
fullPath value set in GDirEntry's constructor, line 613).

Then, GlobalParams::parseNameToUnicode() is called, which is where the 
error I saw came from:
  697   if (!(f = fopen(name->getCString(), "r"))) {
  698     error(-1, "Couldn't open 'nameToUnicode' file '%s'",
  699           name->getCString());
  700     return;
  701   }
Of course the fopen() fails, then that error is printed.

To tell you the truth, I don't know exactly what crashed my program. It 
could be the fopen(), it could be when it tries to concatenate the 
strings together, it could be when it prints the error (or it could be 
something completely different). No matter what though, it's certainly 
an serious bug, and it produces really ugly log messages. I don't yet 
have a full build environment for poppler itself, which is why I kept 
the patch so small and simple.

Hope this helps,

-Adam Batkin