emoon / ProDBG

Debugging the way it's meant to be done
Other
513 stars 31 forks source link

Fix Rust Reader::find_array method #312

Closed SlNPacifist closed 8 years ago

SlNPacifist commented 8 years ago

It does not use result of underlying read_find_array function. It should use this result and return Result like any other find_ function.

Related to at https://github.com/emoon/ProDBG/issues/307

emoon commented 8 years ago

https://github.com/emoon/ProDBG/blob/master/src/plugins/disassembly/src/lib.rs#L49 this code works

SlNPacifist commented 8 years ago

There is no possible way to distinguish if there was no field called "disassembly" or there was array "disassembly" with no elements.

emoon commented 8 years ago

Right but but mixing the names like that is not supported.

SlNPacifist commented 8 years ago

What do you mean by "mixing the names like that"?

emoon commented 8 years ago

having same name for an array or a field. Also the find array returns an iterator (t in the code) which is then inserted back to the ReaderIter struct. Now in the next(..) call for the iterator if this t is not valid/not found it will just return None.

emoon commented 8 years ago

This just allows the Rust code to become a bit nicer to use than the C version as you can start to iterate directly even if next will just skip it directly if nothing was found.

SlNPacifist commented 8 years ago

Here's another problem with current implementation:

    pub fn find_array(&self, id: &str) -> ReaderIter {
        let s = CFixedString::from_str(id).as_ptr();
        let mut t = 0u64;

        unsafe {
            ((*self.api).read_find_array)(transmute(self.api), &mut t, s, 0);
        }

        ReaderIter {
            reader: self.clone(),
            curr_iter: t,
        }
    }

Since result of read_find_array is not used this code always assumes iterator t to be correct. However, if requested field was not found or it was of wrong type, read_find_array will not change t and it will be 0. In this case find_array will yield an invalid iterator. For example, I just replaced

        for reg_data in reader.find_array("registers") {

with

        for reg_data in reader.find_array("aaa") {

and code still magically works.

In my opinion, returning result of find_array should be unambiguous: no field - error wrong field type - error no items in array - empty iterator ok array - non-empty iterator

SlNPacifist commented 8 years ago

Or we could have two functions: a strict one returning errors and loose one returning empty iterator in case of problems.

emoon commented 8 years ago

I'm fine with a more strict one as long as we can keep the usage of the code similar.