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 · 1 comment
Open

String::write_str overflows to WasmRefCell.borrow #4386

osbertngok opened this issue Dec 29, 2024 · 1 comment
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!

@daxpedda
Copy link
Collaborator

daxpedda commented Jan 4, 2025

So my questions are:

  1. Is there any usage above that is not officially supported by wasm-bindgen?

I don't see any. Looks fine to me.

  1. How should I debug the allocation issue above?

I have no idea unfortunately. I'm much more inclined to think there is a bug in wasm-bindgen instead of the allocator. It would probably help if you manage to use source maps to actually get break points in the Rust code as well. See #3698.


If you somehow manage to get it down to a small reproducible example then I would like to take a stab at it myself!

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

2 participants