winreg-rs icon indicating copy to clipboard operation
winreg-rs copied to clipboard

Deleting a key while iterated upon will result in never-ending iterator

Open vthib opened this issue 7 months ago • 0 comments

See for example this reproducer:

#[test]
fn test_delete_key_while_iterating() {
    let hkcu = RegKey::predef(HKEY_CURRENT_USER);

    // First, create the key we will iterate on
    let path = "Software\\WinRegRsTest\\DeleteKeyWhileIterating".to_owned();
    let (key, _disp) = hkcu.create_subkey(&path).unwrap();

    // Then, create two subkeys in it
    key.create_subkey("SubKey1").unwrap();
    key.create_subkey("SubKey2").unwrap();

    // Start iterating on the subkeys.
    let mut iter = key.enum_keys();
    let subkey = iter.next().unwrap();
    assert!(subkey.unwrap().starts_with("SubKey"));

    // Delete the key
    hkcu.delete_subkey_all(path).unwrap();

    // Use the iterator once again. It will never end and return
    // `Some(Err(ERROR_KEY_DELETED))` forever.
    let err = iter.next().unwrap().unwrap_err();
    assert_eq!(
        err.raw_os_error(),
        Some(Foundation::ERROR_KEY_DELETED as i32)
    );
    let err = iter.next().unwrap().unwrap_err();
    assert_eq!(
        err.raw_os_error(),
        Some(Foundation::ERROR_KEY_DELETED as i32)
    );
}

The issue is that the iterator never ends which can result in code hot-looping if they are not careful.

For example, this kind of code will never end:

let subkey_names = key.enum_keys().filter_map(|v| v.ok()).collect();

Of course, you could say that it is by design and the caller should handle errors on his side to decide whether to stop the iteration or keep going. Hence my opening of this issue to discuss it.

I have a PR ready to change the behavior so that on this particular error, the error is first returned, then the iterator returns None. Would it be ok for you?

vthib avatar Jul 25 '24 13:07 vthib