Skip to content

Conversation

ghost
Copy link

@ghost ghost commented Oct 31, 2019

Some architectures, such as ARM, use unsigned 8-bit integers to represent C char type.

In order to make the code work on architectures with both signed and unsigned chars, this PR replaces i8 by c_char in places where it corresponds to C char type.

Signed-off-by: Alexander Rodin <[email protected]>
/// This method is `unsafe` because the pointer must be valid and point to heap.
/// The pointer will be passed to `free`!
pub unsafe fn new_from_i8(message: *const i8) -> Error {
pub unsafe fn new_from_char(message: *const c_char) -> Error {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we're here already, we could also modernise the codebase cough and make this function be pub(crate). Also, this is one of the rare cases where I would be okay with accepting a patch that uses from_utf8_unchecked, as I trust the leveldb maintainers to have their error messages be valid utf8 in all cases.

Copy link
Author

@ghost ghost Oct 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I trust the leveldb maintainers to have their error messages be valid utf8 in all cases

I would not trust them in this, as they are using an ordinary C++ std::string which is not UTF-8 aware. In particular, I was able to generate non-UTF-8 error message with this code on Linux:

use leveldb::{database::Database, options::Options};
use std::{ffi::OsStr, os::unix::ffi::OsStrExt, path::Path};

fn main() {
    let _ = Database::<i32>::open(Path::new(OsStr::from_bytes(b"/\xc3\x28")), Options::new())
        .map_err(|e| println!("{:?}", e));
}

where the path is not valid UTF-8. I've created a separate PR #34 on top of this one that handles non-unicode strings explicitly.

@skade
Copy link
Owner

skade commented Oct 31, 2019

That is correct. Thanks for the patch! I left one comment.

@michaelsproul
Copy link

I believe this could be closed as #36 achieved the same goal.

@skade skade closed this Jun 4, 2020
@skade
Copy link
Owner

skade commented Jun 4, 2020

Closing as suggested. Thanks for the patch nevertheless, @michaelsproul!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants