Skip to content

Commit

Permalink
Replace usage of 'as' keyword
Browse files Browse the repository at this point in the history
It hides that the conversion may be lossy.

This requires breaking ABI for the FFI interface, because the old
behaviour would truncate the number of masters if a plugin had more than
255 (which is not valid in-game, but esplugin could be given an invalid
plugin). This would ultimately cause memory management issues as the
caller wouldn't be able to pass back the full size of the array when it
came to freeing it.

The remaining usages were all to do with comparing with usize values
or indexing, and have been replaced by a function that will panic if the
u32 -> usize conversion will fail but which optimises away on platforms
where usize >= u32 (which is everywhere I expect this code to
run).
  • Loading branch information
Ortham committed Apr 2, 2022
1 parent 501aee3 commit ad4aa93
Show file tree
Hide file tree
Showing 5 changed files with 26 additions and 20 deletions.
4 changes: 2 additions & 2 deletions ffi/src/plugin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ pub unsafe extern "C" fn esp_plugin_filename(
pub unsafe extern "C" fn esp_plugin_masters(
plugin_ptr: *const Plugin,
plugin_masters: *mut *mut *mut c_char,
plugin_masters_size: *mut u8,
plugin_masters_size: *mut size_t,
) -> u32 {
panic::catch_unwind(|| {
if plugin_masters.is_null() || plugin_ptr.is_null() {
Expand All @@ -100,7 +100,7 @@ pub unsafe extern "C" fn esp_plugin_masters(
c_string_vec.shrink_to_fit();

*plugin_masters = c_string_vec.as_mut_ptr();
*plugin_masters_size = c_string_vec.len() as u8;
*plugin_masters_size = c_string_vec.len();

mem::forget(c_string_vec);

Expand Down
2 changes: 1 addition & 1 deletion ffi/tests/ffi.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ void test_esp_plugin_masters() {
assert(return_code == ESP_OK);

char ** masters;
uint8_t num_masters;
size_t num_masters;
return_code = esp_plugin_masters(plugin, &masters, &num_masters);
assert(return_code == ESP_OK);
assert(num_masters == 1);
Expand Down
8 changes: 4 additions & 4 deletions src/form_id.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ use std::cmp::Ordering;
use std::hash::{Hash, Hasher};

use crate::game_id::GameId;
use crate::u32_to_usize;

#[derive(Clone, Debug)]
pub struct HashedFormId {
Expand All @@ -38,14 +39,13 @@ impl HashedFormId {
hashed_masters: &[u64],
raw_form_id: u32,
) -> Self {
let mod_index = raw_form_id >> 24;

let mod_index = u32_to_usize(raw_form_id >> 24);
Self {
overridden_record: (mod_index as usize) < hashed_masters.len(),
overridden_record: mod_index < hashed_masters.len(),
game_id,
object_index: raw_form_id & 0xFF_FFFF,
hashed_plugin_name: hashed_masters
.get(mod_index as usize)
.get(mod_index)
.cloned()
.unwrap_or(hashed_parent_plugin_name),
}
Expand Down
6 changes: 5 additions & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
* You should have received a copy of the GNU General Public License
* along with esplugin. If not, see <http://www.gnu.org/licenses/>.
*/
use std::convert::TryInto;
use std::convert::{TryFrom, TryInto};

pub use crate::error::Error;
pub use crate::game_id::GameId;
Expand All @@ -43,3 +43,7 @@ fn le_slice_to_u32(input: &[u8]) -> u32 {
fn le_slice_to_f32(input: &[u8]) -> f32 {
f32::from_bits(le_slice_to_u32(input))
}

fn u32_to_usize(input: u32) -> usize {
usize::try_from(input).expect("u32 to fit in usize")
}
26 changes: 14 additions & 12 deletions src/record.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ use crate::error::{Error, ParsingErrorKind};
use crate::game_id::GameId;
use crate::record_id::{NamespacedId, RecordId};
use crate::subrecord::{parse_subrecord_data_as_u32, Subrecord, SubrecordRef, SubrecordType};
use crate::u32_to_usize;

const RECORD_TYPE_LENGTH: usize = 4;
pub type RecordType = [u8; 4];
Expand Down Expand Up @@ -64,7 +65,7 @@ impl Record {
game_id: GameId,
expected_type: &[u8],
) -> Result<Vec<u8>, Error> {
let mut content: Vec<u8> = vec![0; header_length(game_id)];
let mut content: Vec<u8> = vec![0; usize::from(header_length(game_id))];
reader.read_exact(&mut content)?;

if &content[0..4] != expected_type {
Expand All @@ -75,9 +76,9 @@ impl Record {
));
}

let size_of_subrecords = crate::le_slice_to_u32(&content[4..]) as usize;
let size_of_subrecords = crate::le_slice_to_u32(&content[4..]);
if size_of_subrecords > 0 {
let mut subrecords = vec![0; size_of_subrecords];
let mut subrecords = vec![0; u32_to_usize(size_of_subrecords)];
reader.read_exact(&mut subrecords)?;

content.append(&mut subrecords);
Expand All @@ -95,12 +96,12 @@ impl Record {
game_id: GameId,
skip_subrecords: bool,
) -> Result<Record, Error> {
let mut header_bytes: Vec<u8> = vec![0; header_length(game_id)];
let mut header_bytes: Vec<u8> = vec![0; usize::from(header_length(game_id))];
reader.read_exact(&mut header_bytes)?;

let header = all_consuming(record_header(&header_bytes, game_id))?;

let mut subrecord_bytes: Vec<u8> = vec![0; header.size_of_subrecords as usize];
let mut subrecord_bytes: Vec<u8> = vec![0; u32_to_usize(header.size_of_subrecords)];
reader.read_exact(&mut subrecord_bytes)?;

let subrecords: Vec<Subrecord> = if !skip_subrecords {
Expand All @@ -125,7 +126,7 @@ impl Record {
let header_length_read = if !header_already_read {
let header_length = header_length(game_id);

header_buffer = &mut header_buffer[..header_length];
header_buffer = &mut header_buffer[..usize::from(header_length)];
reader.read_exact(header_buffer)?;
header_length
} else {
Expand All @@ -135,25 +136,26 @@ impl Record {
let header = all_consuming(record_header(header_buffer, game_id))?;

if game_id == GameId::Morrowind {
let mut subrecords_data = vec![0; header.size_of_subrecords as usize];
let mut subrecords_data = vec![0; u32_to_usize(header.size_of_subrecords)];
reader.read_exact(&mut subrecords_data)?;

let bytes_read = header_length_read as u32 + header.size_of_subrecords;
let bytes_read = u32::from(header_length_read) + header.size_of_subrecords;

let (_, record_id) = parse_morrowind_record_id(&subrecords_data, &header)?;
Ok((bytes_read, record_id))
} else {
// Seeking discards the current buffer, so only do so if the data
// to be skipped doesn't fit in the buffer anyway.
let buffer = reader.fill_buf()?;
if header.size_of_subrecords as usize > buffer.len() {
let usize_of_subrecords = u32_to_usize(header.size_of_subrecords);
if usize_of_subrecords > buffer.len() {
reader.seek(io::SeekFrom::Current(i64::from(header.size_of_subrecords)))?;
} else {
reader.consume(header.size_of_subrecords as usize);
reader.consume(usize_of_subrecords);
}

Ok((
header_length_read as u32 + header.size_of_subrecords,
u32::from(header_length_read) + header.size_of_subrecords,
header.form_id.map(RecordId::FormId),
))
}
Expand Down Expand Up @@ -321,7 +323,7 @@ fn record_id_subrecord_types(record_type: RecordType) -> Vec<&'static SubrecordT
}
}

fn header_length(game_id: GameId) -> usize {
fn header_length(game_id: GameId) -> u8 {
match game_id {
GameId::Morrowind => 16,
GameId::Oblivion => 20,
Expand Down

0 comments on commit ad4aa93

Please sign in to comment.