BlkidProbe.get_value() fails with non-NUL terminated data

[berrange]

The blkid_probe_get_value method can return data without any NUL terminator, hence why its third parameter provides the length of the second parameter.

The BlkidProbe.get_value() wrapper, however, is calling std::ffi::CStr::from_bytes_with_nul and this method is documented thus:

Creates a C string wrapper from a byte slice with exactly one nul terminator.

This function will cast the provided bytes to a CStr wrapper after ensuring that the byte slice is nul-terminated and does not contain any interior nul bytes.

IOW it is not valid to call this with a byte array that may not be NUL-terminated.

This problem can be demonstrated with the following example code

use anyhow::Result;
use libblkid_rs::{BlkidProbe, BlkidProbeRet, BlkidSublks, BlkidSublksFlags};
use std::path::PathBuf;

pub probe(dev: &PathBuf) -> Result<()> {
    let mut probe = BlkidProbe::new_from_filename(&dev)?;
    probe.enable_superblocks(true)?;
    probe.enable_partitions(false)?;
    let flags: Vec<BlkidSublks> = vec![BlkidSublks::Badcsum, BlkidSublks::Magic];
    probe.set_superblock_flags(BlkidSublksFlags::new(flags))?;

    loop {
        match probe.do_probe()? {
            BlkidProbeRet::Success => {
                for idx in 0..(probe.numof_values()?) {
                    let (_name, _value) = probe.get_value(idx as u32)?;
             
                }
            }
            BlkidProbeRet::Done => break,
        };
    }

    return Ok(());
}

When calling this method, pointing it to a LUKS device, it always gets an errror:

Null error when converting from slice: data provided is not nul terminated

because the "SBMAGIC" data is not NUL terminated.

IIUC, fixing this would require changing the signature of get_value so that the returned data is an sequence of bytes not a string.

[jbaublitz]

@berrange This is definitely a bug, but the documentation states the opposite about the null value

/**
 * blkid_probe_get_value:
 * @pr: probe
 * @num: wanted value in range 0..N, where N is blkid_probe_numof_values() - 1
 * @name: pointer to return value name or NULL
 * @data: pointer to return value data or NULL
 * @len: pointer to return value length or NULL
 *
 * Note, the @len returns length of the @data, including the terminating
 * '\0' character.
 *
 * Returns: 0 on success, or -1 in case of error.
 */

We are definitely doing the correct thing here but I need to investigate further whether we're not handling a case that we should be or whether this is a bug I need report upstream to blkid.

[berrange]

I think the docs are a bit ambiguous. They say that 'len' includes the NUL terminator, but doesn't explicitly say there always will be a NUL terminator. Perhaps its also worth a bug report to util-linux first, to clarify their intended semantics for this.

[jbaublitz]

@berrange Can you give me a little bit more information on what the data is for $BMAGIC? Have you debugged this enough to determine what the values of the variables size, name, and data are in our library for the wrapper for that function in your case? After looking at our code, I can see a possibility that size for example is set to zero which would create an empty slice. That would not be null terminated, so we would get the error you're describing. At the very least, we're not checking corner cases like that in our code. Also, digging further into how names and values are created in libblkid, I would say even if that documentation doesn't seem definitive, the allocation for values in libblkid consistently allocates a 0 byte at the end of the buffer and there is a comment that says this should always be the case. I would say that if the data you're getting back is 1. non-empty and 2. non-null terminated, that appears to be a bug in libblkid for sure according to not only the documentation but also the code. However, if it's empty I think we can fix it on our end.

[jbaublitz]

One additional thing that I want to mention is that if the data you're getting back from that value is binary and contains a NULL byte partway through, that would also generate the error you're seeing above even if the binary data is indeed NULL-terminated. I will make a note to clarify that error message so it's a little bit clearer and mentions that other case.

[berrange]

Ok, so I was actually wrong in saying it was a LUKS volume, as I forgot I hadn't encrypted this disk image yet. I'm actually pointing libblkid at a partiition containing an ext4 filesystem.

If I single step blkid_probe_get_value I see the following:

(gdb) print *name
$15 = 0x7ffff7ecc5b6 "SBMAGIC"
(gdb) print *len
$16 = 2
(gdb) print *data
$17 = 0x555555d96d00 <incomplete sequence \357>
(gdb) print (*data)[0]
$20 = 83 'S'
(gdb) print (*data)[1]
$21 = -17 '\357'

IOW, the data is clearly NOT NUL terminated. WIth some more probing I find this magic data is being set from a call blkid_probe_set_magic from superblocks_probe and these 2 bytes of data is indeed the ext4 filesystem superblock magic:

https://github.com/util-linux/util-linux/blob/master/libblkid/src/superblocks/ext.c#L83

I notice that the blkid command line tool never asks for the 'magic' info when probing so never sees this data, which is good as it assuming the data it is getting is NUL terminated too !

I'm suspecting perhaps the SBMAGIC is the only field whose data won't be NUL terminated from blkid_probe_get_value.

Fortunately I don't actually need to access to the magic for my usage, so I can avoid the problem by simply omitting the flag that requests it.

[jbaublitz]

Thanks for the info and digging into this! So are you suggesting that we should open a bug with blkid about this inconsistency? I'm happy to do it or let you do it, whichever you prefer.

[berrange]

So in the linked bug they have confirmed that some values are "raw" and nothing should be assumed about NUL terminators (or embedded NULs)

[jbaublitz]

Okay, in that case I think your suggestion to change the signature to return a byte slice is correct. I'll try to do this soon and we'll put out a minor release.

[jbaublitz]

@berrange Do you know if this affects other "value" methods in other parts of the infrastructure? If you're unsure I can just test it on LUKS2 volumes for all of the methods I find and see what breaks.

[berrange]

I would expect the get_value and lookup_value methods on BlkidProbe to be similarly affected. I don't know offhand if any methods in other classes are affected.

[jbaublitz]

Okay, I'll run some tests.