crystal
crystal copied to clipboard
crystal-lang
Allow mixing of integers and pointers in `Crystal::System.print_error`
When an external library calls Crystal::System.print_error with an argument, we do not have control over the argument type; in the case of Boehm GC, that argument is always an unsigned integer, even though the actual format specifier might be %s or %p. This PR makes those argument types interchangeable. (An integer or pointer is interpreted as the address to a null-terminated string for %s, never the address of a Crystal String.)
Also uses %p consistently in our own calls to .print_error.
Making pointer and integers interchangeable seems like a good idea. That should be safe and sound.
But I'm worried about treating arbitrary integers as string pointers. This is potentially unsafe and can easily crash the process.
I'm not sure if there's a different way to enable the use case with libgc.
Perhaps at least we can restrict %s to work only for the integer type that corresponds to the size of a pointer address? (~ SizeT)
Also I think the refactoring part should be extracted as a separate PR. It's indendent of the designator type related changes.
I don't think restricting the integer type makes the function any safer, because that implies a C library has to indicate the impossibility of pointers by using a 64-bit type on 32-bit systems but a 32-bit type on 64-bit systems.
IMO indicating the impossibility of a pointer is not that relevant. If a lib wants to ensure proper usage, they should prevent improper usage of the designator. My goal would be to make the margin of error as tight as possible. For example, an 8-bit integer type should never be expected to be an address. Or a signed integer type.
That only affects our own calls to .print_error, which are not the focus of this PR. LibGC::Word is going to be the same whether we allow all integer types or just unsigned pointer-sized ones.
Calls from Crystal land might not be the focus of this PR, but it's a use case of this method. And this change indiscriminately enables %s for Int::Primitive. So I think it affects our own calls as well. Unless I'm missing something here?
My concern is about the following program:
Crystal::System.print_error "%s", 1_i8
On master, it prints (???) which I think is correct and expected because an Int8 cannot be a valid address in any realistic scenario (except for zero/null value maybe).
With this patch, it triggers an invalid memory access. That would be avoidable if the type filter was more restrictive than Int::Primitive.
Exactly. I don't see a problem with allowing Int::Primitive here. Otherwise the caller would simply add to_u64 at the call site and that doesn't make the .print_error call any safer.
Yeah, but then it would be explicit. 🤷 Of course we cannot prevent accidental submission of random UInt64 numbers. But we can prevent anything that's not a UInt64 from being erroneously interpreted as a pointer.
You have to include UInt32 since native pointers have that size on 32-bit targets. The most I'd allow is Int32 | UInt32 | Int64 | UInt64 on all platforms, since there is no guarantee C libraries would give us unsigned values.