Skip to content
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

std.os.linux.ifmap contains fields of incorrect width #19980

Closed
weskoerber opened this issue May 16, 2024 · 6 comments · Fixed by #20008
Closed

std.os.linux.ifmap contains fields of incorrect width #19980

weskoerber opened this issue May 16, 2024 · 6 comments · Fixed by #20008
Labels
bug Observed behavior contradicts documented or intended behavior os-linux standard library This issue involves writing Zig code for the standard library.
Milestone

Comments

@weskoerber
Copy link
Contributor

weskoerber commented May 16, 2024

Zig Version

0.13.0-dev.211+6a65561e3

Steps to Reproduce and Observed Behavior

std.os.linux.ifmap is currently defined as:

pub const ifmap = extern struct {
    mem_start: u32,
    mem_end: u32,
    base_addr: u16,
    irq: u8,
    dma: u8,
    port: u8,
};

Note the fields mem_start and mem_end, which are defined as u32s.

On linux, struct ifmap is defined as:

struct ifmap
  {
    unsigned long int mem_start;
    unsigned long int mem_end;
    unsigned short int base_addr;
    unsigned char irq;
    unsigned char dma;
    unsigned char port;
    /* 3 bytes spare */
  };

Note the fields mem_start and mem_end are defined as unsigned long ints. On 64-bit systems, sizeof(unsigned long int) == 8.

On my 64-bit system, the u32s cause incorrect size of std.os.linux.ifmap:

  • @sizeOf(std.os.linux.ifmap) == 32
  • sizeof(struct ifmap) == 40.

This breaks most ioctls to configure network devices. I initially discovered this issue when trying to loop through each network device via the SIOCGIFCONF ioctl.

Expected Behavior

I expect the size of std.os.linux.ifmap to be identical to struct ifmap.

The C standard guarantees long is at least 32 bits but it doesn't, as far as I'm aware, make guarantees about the max size of a long. This causes the 4 byte discrepancy in each field (8 bytes total) between 32-bit and 64-bit architectures.

@weskoerber weskoerber added the bug Observed behavior contradicts documented or intended behavior label May 16, 2024
@Vexu Vexu added standard library This issue involves writing Zig code for the standard library. os-linux labels May 16, 2024
@Vexu Vexu added this to the 0.13.0 milestone May 16, 2024
@weskoerber
Copy link
Contributor Author

I'm not totally sure how to fix this, but am willing to submit a PR. Would the following be appropriate to determine the size of the field?

    // ...
    mem_start: if (@import("builtin").cpu.arch == .x86_64) u64 else u32;
    mem_end: if (@import("builtin").cpu.arch == .x86_64) u64 else u32;
    // ...

Or would it be better to make the fields c_longs?

@clickingbuttons
Copy link
Contributor

I think usize is the appropriate type. C types are to be avoided if possible.

@weskoerber
Copy link
Contributor Author

weskoerber commented May 16, 2024

I think usize is the appropriate type. C types are to be avoided if possible.

Does zig support 16-bit architectures? If so, wouldn't usize still be the incorrect size on 16-bit architectures (i.e. 2 bytes instead of 4)?

@weskoerber
Copy link
Contributor Author

possibly related: #5185

@clickingbuttons
Copy link
Contributor

I may be mistaken, but I don't see a 16 bit target on the tier list.

Additionally, Linux doesn't support 16 bit CPUs. You'll have to use a fork like ELKS which doesn't have a struct ifmap.

All that to say, I think usize is safe. It's used for pointer types everywhere.

@weskoerber
Copy link
Contributor Author

Ah ok, I understand. Thanks for the info!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Observed behavior contradicts documented or intended behavior os-linux standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants