Use strlcpy(3) instead of strncpy(3).

[trasz]

Summary

Use strlcpy(3) instead of strncpy(3); no functional changes.

Sponsored by: NetApp, Inc.
Sponsored by: Klara, Inc.

[imp]

I think strlcpy is wrong. serial_num and device_id are documented as follows:

• serial_num is the device serial number returned in the SCSI INQUIRY VPD
• page 0x80. This should be a unique, per-shelf value. The data inside
• this field should be ASCII only, left aligned, and any unused space
• should be padded out with ASCII spaces. This field should NOT be NULL
• terminated. *
• device_id is the T10 device identifier returned in the SCSI INQUIRY VPD
• page 0x83. This should be a unique, per-LUN value. The data inside
• this field should be ASCII only, left aligned, and any unused space
• should be padded with ASCII spaces. This field should NOT be NULL
• terminated.

Of course the comments should say NUL terminated :).

But then again, I don't see where the generated serial number is the proper length. It's only 12 bytes and the field is 16 bytes.... But other locations will set this field to all spaces.

So I think there's some bugs here, but strncpy is better than strlcpy because it won't drop the last byte of a 16-character serial number.

[trasz]

Interesting. There are two different serial_num fields: one in cbe_lun, and one in params; same with device_id. The cbe_lun->serial_num is indeed documented this way, but params->device_id (ctl_ioctl.h:struct ctl_lun_create_params) is definitely a string; that's how both ctladm(8) and ctld(8) use it.

Now, regarding cbe_lun->serial_num - the code in ctl.c which serializes it into XML handles it as fixed-size string, explicitly passing the length to ctl_sbuf_printf_esc(), which doesn't seem to handle NULs in any special way, it just passes them to sbuf_putc()... I have no idea what sbuf would do with embedded NULs, tbh. And then there's the most important part, which actually talks SCSI, in ctl_inquiry_evpd_serial(); it looks like it returns NUL-filled field, instead of filling it with spaces as required by the spec.

Also, whatever we end up with, a similar change should probably happen to ctl_backend_ramdisk.c.

[mav]

I agree with Warner that the change in this form is not needed. It is likely that some cleanup may be needed there to honor some more specification details (IIRC there are some padding requirements in VPDs that may be not honored now), but that should be done very carefully to not affect already running systems.

[scottl]

Yeah, my reading is that this code is not compliant with the spec, and that changing strncpy() to strlcpy() won't help. What I would probably do is something list this:

u_int tmplen;
tmplen = snprintf(tmpstr, sizeof(tmpstr), "MYSERIAL%04d", softc->num_luns);
if (tmplen > sizeof(tmpstr) {
    ... error ...
}
memset(tmpstr + tmplen, sizeof(tmpstr) - sizeof(tmpstr), 0x20);

Then I'd use memcpy() to copy tmpstr into the places where no NULL termination is allowed, and snprintf() to copy it into the strings where NULL termination is required.

Then I'd get a stiff drink and see if this can be done more sanely in Rust =-)

[trasz]

There's a consensus that this change doesn't improve the state of things; drop it for now.