Skip to content

Implement if_nametoindex for windows #22555

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

Merged
merged 5 commits into from
Apr 4, 2025
Merged

Conversation

TheOnAndOnlyZenomat
Copy link
Contributor

@TheOnAndOnlyZenomat TheOnAndOnlyZenomat commented Jan 20, 2025

This implements the if_nametoindex function for windows.

I noticed zig threw a compile error when trying to compile the zigdig example code for windows.
If I read the code correctly, and looking at issue #20530, this seems also hinder all IP resolution on windows, even though if_nametoindex is only called in the ipv4 codepath.

I pieced this code together from looking at the darwin implementation and the old std.x namespace, which also contained an implementation for windows.

This seems to pass all test, except for this one

try testing.expectError(error.Overflow, net.Address.resolveIp6("ff01::fb%wlp3s0s0s0s0s0s0s0s0", 0));
. As far as I understand, the error.Overflow case should be handled before if_nametoindex is even called. If someone can help me fix this case, I'd be happy for any pointers.

I'm also happy about any other pointers and things I should fix.

Fixes #20530.

@andrewrk andrewrk enabled auto-merge (squash) January 21, 2025 03:00
@andrewrk
Copy link
Member

Thanks!

@alexrp
Copy link
Member

alexrp commented Jan 29, 2025

CI failures look related to me.

@alexrp alexrp disabled auto-merge January 29, 2025 07:50
@TheOnAndOnlyZenomat
Copy link
Contributor Author

I'd be happy to merge any changes or fixes, if that is something that would be beneficial. Anything else I can contribute to this?

@alexrp
Copy link
Member

alexrp commented Jan 29, 2025

Well one of the tests you enabled to run on Windows is failing there:

test
+- test-modules
   +- test-std
      +- run test std-x86_64-windows.win11_ge...win11_ge-gnu-skylake-Debug-libc 2690/2751 passed, 1 failed, 60 skipped
error: 'net.test.test.parse and render IPv6 addresses' failed: expected error.Overflow, found error.InterfaceNotFound
C:\Users\ci\george5\_work\zig\zig\lib\std\fmt.zig:1768:24: 0x7ff7ce2536c0 in charToDigit (test.exe.obj)
    if (value >= base) return error.InvalidCharacter;
                       ^
C:\Users\ci\george5\_work\zig\zig\lib\std\fmt.zig:1633:23: 0x7ff7ce231c4f in parseIntWithSign__anon_14207 (test.exe.obj)
        const digit = try charToDigit(math.cast(u8, c) orelse return error.InvalidCharacter, buf_base);
                      ^
C:\Users\ci\george5\_work\zig\zig\lib\std\fmt.zig:1521:5: 0x7ff7ce28132e in parseIntWithGenericCharacter__anon_23202 (test.exe.obj)
    return parseIntWithSign(Result, Character, buf, base, .pos);
    ^
C:\Users\ci\george5\_work\zig\zig\lib\std\fmt.zig:[150](https://github.com/ziglang/zig/actions/runs/12868010238/job/35904763031?pr=22555#step:3:151)8:5: 0x7ff7ce2719cf in parseInt__anon_21549 (test.exe.obj)
    return parseIntWithGenericCharacter(T, u8, buf, base);
    ^
C:\Users\ci\george5\_work\zig\zig\lib\std\net.zig:764:13: 0x7ff7cf446160 in if_nametoindex (test.exe.obj)
            return error.InterfaceNotFound;
            ^
C:\Users\ci\george5\_work\zig\zig\lib\std\net.zig:613:28: 0x7ff7cf44438d in resolve (test.exe.obj)
                break :blk try if_nametoindex(scope_id_str);
                           ^
C:\Users\ci\george5\_work\zig\zig\lib\std\net.zig:103:26: 0x7ff7cf442956 in resolveIp6 (test.exe.obj)
        return .{ .in6 = try Ip6Address.resolve(buf, port) };
                         ^
C:\Users\ci\george5\_work\zig\zig\lib\std\testing.zig:51:13: 0x7ff7cf441c1b in expectError__anon_1173136 (test.exe.obj)
            return error.TestUnexpectedError;
            ^
C:\Users\ci\george5\_work\zig\zig\lib\std\net\test.zig:79:9: 0x7ff7cf44262d in test.parse and render IPv6 addresses (test.exe.obj)
        try testing.expectError(error.Overflow, net.Address.resolveIp6("ff01::fb%wlp3s0s0s0s0s0s0s0s0", 0));
        ^

So that needs to be resolved.

@TheOnAndOnlyZenomat
Copy link
Contributor Author

I also saw that. What I didn't get: To me it seems like the codepath where the error.Overflow is emitted happens before if_nametoindex is even called. And no other implementation of if_nametoindex handles the error.Overflow case. And I didn't touch any code outside that.

@tensorush
Copy link
Contributor

tensorush commented Jan 29, 2025

Seems that this condition doesn't trigger:

zig/lib/std/net.zig

Lines 530 to 532 in 0679358

if (scope_id_index >= scope_id_value.len) {
return error.Overflow;
}

Because when running on Windows with libc posix.IFNAMESIZE resolves to being 30, instead of being 16 like on Linux/Darwin:

zig/lib/std/c.zig

Lines 5783 to 5790 in 0679358

pub const IFNAMESIZE = switch (native_os) {
.linux => linux.IFNAMESIZE,
.emscripten => emscripten.IFNAMESIZE,
.windows => 30,
.openbsd, .dragonfly, .netbsd, .freebsd, .macos, .ios, .tvos, .watchos, .visionos => 16,
.solaris, .illumos => 32,
else => void,
};

So I'd suggest making that test string 12 characters longer (scope_id_value starts after %), covering the maximum posix.IFNAMESIZE value of 32 (Solaris/Illumos) just in case:

"ff01::fb%wlp3s0s0s0s0s0s0s0s0s0s0s0s0s0s0"

@TheOnAndOnlyZenomat
Copy link
Contributor Author

I was also looking at that piece of code, but didn't make the IFNAMESIZE connection. I could update the test case tomorrow, rebase the latest changes and push again, if this is the accepted solution

Windows has a different max length for their interface names,
leading to test cases not having the expected result
Copy link
Contributor

@tensorush tensorush left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems that the "ff01::fb%12345678901234" test string, since its scope id was only 14 characters long, was triggering one of the different error.Overflows that happen later on:

zig/lib/std/net.zig

Lines 584 to 597 in f954764

} else {
const digit = try std.fmt.charToDigit(c, 16);
{
const ov = @mulWithOverflow(x, 16);
if (ov[1] != 0) return error.Overflow;
x = ov[0];
}
{
const ov = @addWithOverflow(x, digit);
if (ov[1] != 0) return error.Overflow;
x = ov[0];
}
saw_any_digits = true;
}

Revert a change, as that was supposed to target a different test case
@jklw10
Copy link

jklw10 commented Mar 23, 2025

worked for me in latest master build 0.15.0-dev.79+9f235a105 please merge 👍

@alexrp alexrp requested a review from squeek502 March 24, 2025 15:08
Copy link
Collaborator

@squeek502 squeek502 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The if_nametoindex docs say:

The if_nametoindex function is implemented for portability of applications with Unix environments, but the ConvertInterface functions are preferred. The if_nametoindex function can be replaced by a call to the ConvertInterfaceNameToLuidA function to convert the ANSI interface name to a NET_LUID followed by a call to the ConvertInterfaceLuidToIndex to convert the NET_LUID to the local interface index.

Not sure it's worth worrying about for now, though. At some point I assume it'd be better to swap things over to that (or ConvertInterfaceNameToLuidW but I'm unsure if interface names can be non-ASCII?) and also use GetAddrInfoW instead of getaddrinfo (unrelated to this PR, was just looking at other ws2_32 calls)

(or lower-level equivalents if they exist, I didn't see any ntdll calls when running if_nametoindex with NtTrace [note, though, that it doesn't capture Rtl function calls])

@alexrp
Copy link
Member

alexrp commented Mar 26, 2025

Does this actually fix #20530?

@squeek502
Copy link
Collaborator

squeek502 commented Mar 26, 2025

Does this actually fix #20530?

Yes, it fixes the compile error in that issue.

> zig run 20530.zig
Started http server on port 4221

The tests that call resolveIp6 effectively cover it, since the error was from resolveIp referencing resolveIp6 which would hit the compile error.

@alexrp alexrp added this to the 0.14.1 milestone Mar 26, 2025
@alexrp alexrp enabled auto-merge (squash) April 4, 2025 04:10
@TheOnAndOnlyZenomat
Copy link
Contributor Author

Are those CI failures related? I saw some similar ones in different PR's, and I don't think that the string concatenation in the test cast should cause a OOM during comptime

@alexrp
Copy link
Member

alexrp commented Apr 4, 2025

Nope, those aren't your fault. The PR should be good to go; I'll merge it once we resolve those CI issues.

@TheOnAndOnlyZenomat
Copy link
Contributor Author

Alright. Thanks everyone for working through this with me :D

@alexrp alexrp merged commit 8a83fc7 into ziglang:master Apr 4, 2025
16 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trying to call std.net.Address.resolveIp fails on windows with a compiler error
6 participants