The implementation can/should be more generic, and play nicer with lifetimes

[le-jzr]

The comment in the function tries to explain safety with "The multiboot specification defines the module str as valid utf-8, therefore this function produces defined behavior". Such trust in constraint assumed by specification is a horribly wrong thing to do.

It might not matter much when the data is used by kernel to boot, as corrupted data there automatically break all safety, but that makes the implicit assumption that the crate will never, ever, ever, be used to access multiboot data in other contexts, e.g. a memory dump for debugging.

[le-jzr]

Well, I guess the entire implementation has that assumption. Mind if I start another crate that tries to be a bit more generic? :)

[4e554c4c]

Good point. We rely on the multiboot specification to be correct in multiple contexts and it would probably be best to assume that data can be malformed. The most dangerous assumptions, in my opinion, are not assuming utf8 is correct, but trusting size sections, which could easily lead to segfaults or worse in a kernel context.

However in several cases it may be difficult for the crate to understand what data is "correct" and safe. Short of saving the size of the data structure and verifying all pointers to be in this range, what can be done? Such bookeeping could limit the way that this crate could be used, especially in contexts such as higher half kernels.

[le-jzr]

To elaborate on my previous comment, Multiboot is particular in that the structures are linked together with physical addresses, and the crate basically interprets them all as raw pointers. Alternative is to abstract away the memory as a random-access file and let user decide how those reads are fulfilled. That imo could make the crate cleaner and less riddled with unsafe blocks.

[4e554c4c]

@Zarevucky That sounds like a good idea! Treating data in this way has created problems before as well. It may make sense to create your own crate because the small scope of this one, however if you have ideas on how to achieve these goals feel free to impliment them here.

[phil-opp]

Well, I guess the entire implementation has that assumption. Mind if I start another crate that tries to be a bit more generic? :)

Not at all! I don't have the time to maintain this crate anyway, so I would be happy to switch to something better. Of course, we could also reuse this crate, since semantic versioning allows us to do breaking changes. I just gave both of you push access, feel free to do any refactoring and restructuring you want!

[ahmedcharles]

The kernel has no choice but to trust that the boot loader is correctly implemented, at some level. The point of multiboot as a specification is, in part, to allow the kernel to push work into the boot loader and make the kernel simpler. Obviously, there is some validation that can occur, but I don't really see the point, given that once 32-bit protected mode is on, the only way to know how much memory is in the system is to use the multiboot structure, since using the bios is no longer an option. And other than checking the multiboot information structure for internal consistency, the first interesting check would be whether it fits in memory or not. And checking that would require going back into real mode (Rust doesn't even help there) or trusting the multiboot information structure (which is what we're trying to verify).

I've considered what this library might look like with less assumptions about the underlying data being valid, but in the end, I think it would just make it harder to write, read and maintain, while (like I said above) not providing any interesting benefits. If the boot loader is broken, the kernel has no hope of recovering.

[ahmedcharles]

And the distrust of specifications is interesting, given that the only reason Box::new(42) can be considered safe is because the specification for malloc (or whatever other system function is used for allocation) says that requesting X bytes of memory will actually result in that much memory being valid at the returned address (unless the address is null) and that the memory won't be aliased with other memory returned by the same function. And note, most linux systems don't guarantee that allocated memory is valid, but most programs work anyways (until the oomkiller kills them, I guess).

Similar with instruction sets and how processors work, relying on specifications.

So, if we can rely on xor eax, eax actually setting eax to 0, why can we not rely on a multiboot compliant boot loader to create the correct structure in memory? (And there are examples of intel shipping specific processors that behave incorrectly and that doesn't change the fact that we trust them to work correctly, in general)

[le-jzr]

@ahmedcharles Please read what I wrote before responding. Repeating myself: while data provided by the bootloader must be assumed correct, it's quite plausible that the data might be interpreted in other contexts, like memory dump for debugging, and similar. Also repeating myself, current implementation assumes only one context of operation, and that in my opinion is just bad practice. Opinions may differ on this, but your last two comments explicitly dismantle arguments that nobody made.

[le-jzr]

To further explain my point of view, consider this: Lots of the unsafe conversions in current code assign 'static lifetime to references. This means that from Rust point of view, you are never allowed to unmap/remap the initial memory mapping. This restriction is never documented anywhere. Whether or not the underlying data is correct, the code just steamrolls through all the assurances Rust is trying to make by unsafe blocks everywhere. That unsafety is easily extracted behind a safe abstraction (in progress), while current code is much like C code with Rust syntax.

[ahmedcharles]

I reread your previous statements and would still respond in the same way, so perhaps I just don't understand. And your new statements just make it more confusing. The only reason I'm interested is because I'm planning changes to make the crate usable with kernels that are relocated to the higher half of memory using paging and that requires special care when interpreting the multiboot information.

I suspect that talking about specific code changes will be more fruitful.

[le-jzr]

I have to admit I am now confused myself. You don't disagree that using the crate to read data from memory dump file is something we might want to allow? Then your arguments about specifications are irrelevant, since data parsing must always assume incorrect or even malicious data. You could hardly build a safe file parser by assuming that the file follows specification without fail.

[le-jzr]

The larger point I was trying to make is that the code is in haphazard ignorance of Rust safety rules. I have only been learning Rust for a few weeks (admittedly with background in formal verification, but still), and even I can see that this crate is currently more C than Rust.

[le-jzr]

That's not meant in a bad way. I'm just having trouble understanding where you are coming from, because the safety hazards in current code seem obvious to me, as is the assumption that the principles of defensive programming have a valid place in kernel code.

[le-jzr]

I suspect that talking about specific code changes will be more fruitful.

Working on it! Stay tuned.

As mentioned above, new to Rust, so the lifetime annotations are giving me a hard time.