[Bug 6767] failure enumerating interfaces on NetBSD

[wine-bugs at winehq.org]

http://bugs.winehq.org/show_bug.cgi?id=6767





--- Comment #6 from jmmikkel at mit.edu  2007-11-26 23:47:02 ---
Hi, I apologize for having dropped the ball back when. Then, this past June I
set out to deal with this and found that the code had changed. Unfortunately
the current code will still not work correctly, and the application I was using
when this problem came up for me no longer worked but for a different reason
(it does not even get this far). I didn't have another application that used
this functionality to try with, and I have not managed to sort through it since
then.

(I do use the pkgsrc build, although it would appear 0.9.39 compiled without
pkgsrc patches in June.)

The problem with the current version is that NetBSD places the required length
in ifc->ifc_len at all times. So it will always have the same value two ioctl
calls in a row, even if the provided buffer is still too small the second time.

Because I don't have an easy way to test this with an actual application in
Wine, I have copied the enumIPAddresses function to a separate file, and added
some printing, so I can test with just that. When I run the existing code, I
find that ifc->ifc_len starts at 128, and the ioctl sets it to 352. Since
ifc->ifc_len != lastlen (352 != 0) it goes through the loop again, setting
lastlen to 352 and ifc->ifc_len to 256. The ioctl once again sets it to 352 and
the loop terminates, but it shouldn't have, because 352 > 256 so my list will
be truncated.

I don't know why it was changed to do the "two in a row" test. I can't think of
a way for the ioctl to work that would make that test necessary instead of
something more like the >= test I originally suggested. The ioctl can put three
kinds of numbers in ifc_len. Numbers smaller than the provided buffer indicate
the total length of the data, including zero. This means that for cases where
the buffer is not big enough, there are only two possible choices:
1.) ioctl() returns the buffer size if the buffer is too short (Linux behavior,
IIRC)
2.) ioctl() returns a number bigger than the size if the buffer is too short
(NetBSD -- we actually need not assume the number is accurate).

The twice-in-a-row solution we currently have always results in an extra trip
through the loop unless the actual length of the interface list is zero,
because lastlen starts out at zero. Consider a required length of 100. lastlen
is set to 0, the buffer is allocated with size 128, and the ioctl sets the
required length to 100. This is != 0 so we loop again, setting lastlen to 100
and allocating a buffer of size 256. The ioctl sets the length to 100 again and
the loop terminates, having run twice instead of once.

At any rate, the solution certainly covers situation 1: 128 is < 352 so the
ioctl sets the length to 128, and the loop sets lastlen to 128. The ioctl is
called with a buffer of 256 and ioctl sets the length to 256, which is != 128,
so we loop again and call with a buffer of 512. The ioctl sets the length to
352, which is != 256, and we call the ioctl again with a buffer of 1024 but
then terminate.

The twice-in-a-row solution doesn't cover situation 2 if the required length is
> 256 as I described above. It works okay when the length is less than 256.
Perhaps this is the situation that was addressed with the change to the current
algorithm? Oddly, the solution also works if the ioctl does something strange
like fill in the provided size + 1 when the buffer is not big enough (buffer
size 128 -> ifc_len set to 129, buffer size 256 -> ifc_len set to 257, buffer
size 512 -> ifc_len set to 352, buffer size 1024 -> ifc_len set to 352). But it
breaks if the ioctl is *consistent* about the size.

A >= test also covers situation 1, at the price of an extra trip through the
loop but only when the required buffer size is exactly what was provided. But
that corner case required an extra trip in the first version I worked with too
(using the == test you can see in the diff in the initial report). If 352 is
required and 128 is provided, the ioctl sets the length to 128 and 128 >= 128
so the next loop the buffer size is 256 and the ioctl sets it to 256 and 256 >=
256 so the next loop the buffer size is 512 and the ifc_len is set to 352,
which is not >= 512 and the loop terminates.

The >= test covers situation 2 as well, including for required buffer lengths >
256, of course, since it's the fix that worked for me. For NetBSD we would call
the ioctl with a buffer size of 128, the ifc_len would be set to 352, and 352
>= 128 so the next time through the loop would call ioctl with a size of 256,
ifc_len would be set to 352, which is >= 256, so the next loop the buffer size
is 512 and the ioctl sets ifc_len to 352, which is not >= 512. 

This also works with the strange version of the ioctl that returns the size + 1
when the provided size is too small: buffer size 128 -> ifc_len set to 129 and
129 >= 128 so now buffer size 256 -> ifc_len 257 and 257 >= 256 so then buffer
size 512 -> ifc_len set to 352, which is not >= 512 and the loop terminates.

So my suggestion would be to return the loop back to how it was a year ago but
with >= instead of == (removing lastlen):

    ret = NO_ERROR;
    ifc->ifc_len = 0;
    ifc->ifc_buf = NULL;
    /* there is no way to know the interface count beforehand,
       so we need to loop again and again upping our max each time
       until returned < max */
    do {
      HeapFree(GetProcessHeap(), 0, ifc->ifc_buf);
      if (guessedNumAddresses == 0)
        guessedNumAddresses = INITIAL_INTERFACES_ASSUMED;
      else
        guessedNumAddresses *= 2;
      ifc->ifc_len = sizeof(struct ifreq) * guessedNumAddresses;
      ifc->ifc_buf = HeapAlloc(GetProcessHeap(), 0, ifc->ifc_len);
      ioctlRet = ioctl(fd, SIOCGIFCONF, ifc);
    } while (ioctlRet == 0 &&
     ifc->ifc_len >= (sizeof(struct ifreq) * guessedNumAddresses));

But if the twice-in-a-row test is really necessary for some reason I'm not
seeing here, we could perhaps patch up NetBSD with the following diff to the
current version, which will take advantage of the provided required length
without changing the algorithm for OSes that leave the provided buffer size as
the length when it's long enough. It seems more hackish to me, though (but it
does eliminate the extra trip through the loop when the required size is >=
128!).

--- dlls/iphlpapi/ifenum.c.orig
+++ dlls/iphlpapi/ifenum.c
@@ -695,6 +695,9 @@
       else
         guessedNumAddresses *= 2;
       ifc->ifc_len = sizeof(struct ifreq) * guessedNumAddresses;
+      if (lastlen > ifc->ifc_len) {
+       ifc->ifc_len = lastlen;
+      }
       ifc->ifc_buf = HeapAlloc(GetProcessHeap(), 0, ifc->ifc_len);
       ioctlRet = ioctl(fd, SIOCGIFCONF, ifc);
     } while ((ioctlRet == 0) && (ifc->ifc_len != lastlen));

Sorry I can't test this with the application I was using any more (I'd have to
debug some other thing that has gone wrong in the last year first) but I can
easily test out whatever change is decided upon with my little standalone
tester.


-- 
Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching all bug changes.