Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

String::write_str overflows to WasmRefCell.borrow #4386

Open
osbertngok opened this issue Dec 29, 2024 · 0 comments
Open

String::write_str overflows to WasmRefCell.borrow #4386

osbertngok opened this issue Dec 29, 2024 · 0 comments
Labels

Comments

@osbertngok
Copy link

osbertngok commented Dec 29, 2024

Summary

I am building a relatively simple wasm using wasm-bindgen 0.2.99 and run is via Vite-React on Chrome, where I expose the following rust structs to JS through wasm-bindgen:

#[wasm_bindgen]
pub struct ChordProgressionManager {
    config: WASMConfig,
}

#[wasm_bindgen]
impl ChordProgressionManager {
    #[wasm_bindgen(constructor)]
    pub fn new() -> Self {
        // Create a default Chord Progression Manager
        ChordProgressionManager {
            // The following preset is from 00-4-part-writing-with-triads-and-7ths
            config: WASMConfig {
                numOfSequentialChords: 5,
                minNumOfPitches: 3,
                maxNumOfPitches: 4,
                minPitch: 40,
                maxPitch: 76,
                minThickness: 0.0,
                maxThickness: 50.0,
                minRoot: 0,
                maxRoot: 11,
                minGeometryCenter: 0.0,
                maxGeometryCenter: 70.0,
                minPitchClassSetSize: 3,
                maxPitchClassSetSize: 4,
                minCOFSpan: 0,
                maxCOFSpan: 12,
            },
        }
    }
    pub fn loadConfig(self: &mut Self, js_obj: &JsValue) -> bool {
        match WASMConfig::from_js_object(js_obj) {
            Ok(conf) => {
                self.config = conf;
            }
            Err(err) => {
                web_sys::console::error_1(&err);
                return false;
            }
        }
        return true;
    }

    pub fn calculateChordProgression(self: &Self) -> String {
        // Just in case there is an unexpected panic
        console_error_panic_hook::set_once();

        // Step 1: convert WASMConfig to CNConfig
        let cnConfig: CNConfig = self.config.clone().into();
        // Step 2: calculate the initial chord and return.
        let min_pitch = match Pitch::from_midi(cnConfig.min_pitch as u8) {
            Ok(mp) => mp,
            Err(_) => return GenerateRandomChordError::InvalidPitchRange(format!("Cannot convert into Pitch using min_pitch {}", cnConfig.min_pitch)).to_string(),
        };
        let max_pitch = match Pitch::from_midi(cnConfig.max_pitch as u8) {
            Ok(mp) => mp,
            Err(_) => return GenerateRandomChordError::InvalidPitchRange(format!("Cannot convert into Pitch using max_pitch {}", cnConfig.max_pitch)).to_string(),
        };
        match generate_random_chord(
            cnConfig.min_num_of_pitches,
            cnConfig.max_num_of_pitches,
            min_pitch,
            max_pitch,
        ) {
            Ok(chord) => format!("Initial Chord is {}", chord),
            Err(err) => format!("Error generating initial chord. {}", err),
        }
    }
}

The wasm runs just fine, except sometimes I got memory out of bound, recursive use of an object detected which would lead to unsafe aliasing in rust, table index out of bound exceptions.

At first I thought it was certain borrow rules were not observed in my JavaScript code, that I really tried to borrow_mut from an object that was already borrowed. Then I realised it didn't make sense because I am not using webWorker, and I don't have async code in my rust codebase, so this shouldn't happened.

Then I drilled down on the prominent error recursive use of an object detected which would lead to unsafe aliasing in rust, and it turned out that it was complaining because the borrow count is not 0, and strangely, every time when it fails, the borrow count is a constant (1919903810), but it is far from i32::MAX, so it wasn't a recursive mute borrow.

pub struct WasmRefCell<T: ?Sized> {
    borrow: Cell<usize>,
    value: UnsafeCell<T>,
}

Which leaves the possibilities that either the ChordProgressionManager was indeed readonly-ly borrowed 1919903810 times, or there were some illegal memory copies brewing. After some painstakingly breakpoint witch-hunts, I finally pinpoint the unexpected memory copy occurs in func $<alloc::string::String as core::fmt::Write>::write_str::hf0fb92960288353a, where there is a call $memcpy. Specifically, it was copying 17 characters "Initial Chord is " to the data field of a String, so I assume somehow there is a String's allocation incorrectly overlaps with WasmRefCell of the ChordProgressionManager.

Array.from(new TextEncoder().encode('Initial Chord is '.substr(8,4))).map(byte => byte.toString(16).padStart(2, '0')).reverse().join('')
'726f6843'
(1919903810).toString(16)
'726f6842' // the diff of 1 was due to drop is called and counter is subtracted by one

So my questions are:

  1. Is there any usage above that is not officially supported by wasm-bindgen?
  2. How should I debug the allocation issue above?

Let me know if more details is needed. Thanks in advance on spending time looking into this writeup!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant