Return something other than `error.Unexpected` from `std.os.windows.QueryObjectName` for unnamed objects.

[Trevor-Strong]

Zig Version

0.14.0-dev.2547+77c63ac36

Steps to Reproduce and Observed Behavior

const std = @import("std");
const w = std.os.windows;

extern "kernel32" fn CreateSemaphoreW(
    security_attrs: ?*w.SECURITY_ATTRIBUTES,
    init_count: i32,
    max_count: i32,
    name: ?[*:0]const u16,
) callconv(.winapi) ?w.HANDLE;

pub fn main() !void {
    // Create an unnamed semaphore
    const sem = CreateSemaphoreW(null, 1, 1, null) orelse unreachable;
    var buffer: [w.PATH_MAX_WIDE]u16 = undefined;
    const result = w.QueryObjectName(sem, &buffer));
    // 'isUnnamed' is impossible to implement right now but it really should be
    if (isUnnamed(result)) {
        std.debug.print("Unamed\n", .{});
    } else {
        const name = try result;
        std.debug.print("Sem name: \"{}\"", .{ std.unicode.fmtUtf16Le(name) });
    }
} 

Expected Behavior

There should be some way to detect if the handle given to QueryObjectName is for an unnamed object or at the very least it should not return error.Unexpected for a condition this is very much possible (as noted in the QueryObjectName implementation).

The comment mentioned appears to handle it this way because QueryObjectName was originally written (I guess) to support file name lookup; however, the function should not limit itself to this use case as there are other things that can be named besides files and even files are sometimes unnamed, such as anonymous pipes.

[squeek502]

All returns of error.Unexpected in this context are indicative of a missing error mapping.

zig/lib/std/posix.zig

Lines 7479 to 7484 in 055969b

	/// Whether or not `error.Unexpected` will print its value and a stack trace.
	///
	/// If this happens the fix is to add the error code to the corresponding
	/// switch expression, possibly introduce a new error in the error set, and
	/// send a patch to Zig.
	pub const unexpected_error_tracing = builtin.zig_backend == .stage2_llvm and builtin.mode == .Debug;

Could you post the full error that includes the NTSTATUS that's being mapped to error.Unexpected?

[Trevor-Strong]

@squeek502 This problem is caused by this line here.

This instance of returning error.Unexpected conveniently doesn't call unexpectedStatus or any other helper function that prints out a trace according to std.posix.unexpected_error_tracing since it maps a particular value to error.Unexpected when the function return STATUS_SUCCESS, so there is no trace for me to give.

Right above this line, there is comment about a zeroed UNICODE_STRING being used to indicate an unnamed object, which I also linked to in the original issue.

For me the problem isn't that this is an error condition rather that this condition is not detectable, since error.Unexpected is returned.

[squeek502]

Ah, ok, sorry; thanks for the clarification.

Some potential fixes:

• Return a zero-length slice
• Make the return value an optional and return null
• Return a relevant error like error.UnnamedObject or something like that

Not sure which would be best.

[Trevor-Strong]

I'm most in favor of returning an different error (such as error.UnnamedObject). This is an exceptional condition, and the function is called, QueryObjectName, so if the object doesn't have a name it makes sense for that to be reported as an error.

[Trevor-Strong]

After making my own version of QueryObjectName that handles unnamed objects and using it, I've actually reversed my opinion. I can see a lot more value in returning null or an empty string instead of an error than I did initially.

In the case you actually want to handle an unnamed object (which is pretty normal for everything that's not a regular file or directory), returning an error makes the handling look like this:

const name = QueryObjectName(handle, buffer) catch |err| switch (err) {
    error.UnnamedObject => // Do thing for unnamed object
    // handle other errors
};
// Do thing for named object

which is pretty inconvenient since I found that I usually wanted my catch to basically be
"if unnamed do thing else return err" especially when compared to this example where null is returned:

if (try QueryObjectName(handle, buffer)) |name| {
    // Do thing for named object
} else {
    // Do thing for unnamed object
}

Returning an empty string isn't quite as nice as the null case, but still lets you skip the extra 3-4 lines of catch handling required if error.UnnamedObject is used instead.

And in the case where you do want to treat an unnamed object as an error condition, it's really easy to
just do try QueryObjectName(handle, buffer) orelse return error.UnnamedObject or

const name try QueryObjectName(handle, buffer);
if (name.len == 0) return error.UnnamedObject
// Do thing

If an empty string is returned instead.

[squeek502]

Returning an optional seems good in that it forces the caller to handle the unnamed case in some way, whereas with the empty string it seems like you could accidentally mishandle the unnamed case.

[Trevor-Strong]

Agreed