Elf: NoteIterator has an infinite loop

[m4b]

When parsing the multiarray.so binary mentioned in #106, the NoteIterator appears to infinitely loop if we do something like:

        if let Some(notes) = self.elf.iter_note_headers(self.bytes) {
            fmt_hdr(fmt, "Notes")?;
            writeln!(fmt, "")?;
            for (i, note) in notes.enumerate() {
                if let Ok(note) = note {
                    fmt_idx(fmt, i)?;
                    write!(fmt, " ")?;
                    fmt_str(fmt, note.name.trim_right_matches('\0'))?;
                    write!(fmt, " type: {} ", note.type_to_str())?;
                    for byte in note.desc {
                        write!(fmt, "{:x}", byte)?;
                    }
                    writeln!(fmt, "")?;
                } else {
                    writeln!(fmt, "BAD NOTE: {:?}", note);
                }
            }
            writeln!(fmt, "")?;
        }

[m4b]

@philipc sorry, it wasn't clear; were you indicating this was a bug in us, or in clients using the note iterator, for when it loops infinitely? I'm inclined to believe it's generally almost always a bug in us if any method of ours infinitely loops, but perhaps this is one of those special cases?

[philipc]

I think it's better to make it hard to misuse an API (eg to cause an infinite loop), than to rely on clients to invoke it correctly. So fixing this in goblin would be great.

We probably should audit all iterators. I changed the code in the object crate to handle all potential infinite loops, without checking if goblin already handled those correctly.

Also, just as information (I'm not necessarily suggesting that goblin change to this), in gimli we changed our iterators to return Result<Option<_>> instead of Option<Result<_>>, so that it's harder for clients to write code that would infinitely loop, or ignore the error. The downside of that is you can no longer use rust's for loop sugar to iterate, and use of Option<Result<_>> is more widespread in rust so this is perhaps unidiomatic. We still make the iterators set their remaining length to 0 when there is an error though.

[m4b]

Auditing sounds good to me; will have to evaluate pros and cons of switching as you mention. I’d also like to fix goblin parse return to be more ergonomic.

But yea agreed we should fix if can. Not super pressing atm tho I think, but def before 1.0 :)

[anfedotoff]

Hello, in my case there is a hang too. Here is the code to reproduce:

use goblin::{error, Object};
use std::path::Path;
use std::env;
use std::fs;
use goblin::elf::header;

fn main() -> error::Result<()> {
    for (i, arg) in env::args().enumerate() {
        if i == 1 {
            let path = Path::new(arg.as_str());
            let buffer = fs::read(path)?;
            match Object::parse(&buffer)? {
                Object::Elf(elf) => {
                    if elf.header.e_type == header::ET_CORE {
                        let notes_iter = elf.iter_note_headers(&buffer);
                        if let Some(notes_iter) = notes_iter {
                            println!("Notes count:{}", notes_iter.count())
                        }
                    }
                },
                _ => {

                }
            }
        }
    }
    Ok(())
}

Here is the core file to parse (file is broken, it is generated by testing tool) file_core_arm_pi.zip Hang is occurred in function:

 elf.iter_note_headers(&buffer);

Execution doesn't get to the next line.

[philipc]

The hang is actually on notes_iter.count(). So yes it's the same problem as described in this issue: currently you must not continue iterating after an error occurs or you'll get an infinite loop (and count will always continue iterating).

[philipc]

By the way, that file_core_arm_pi.zip has a lot of stuff wrong with it (e.g. try running readelf on it). So while goblin should do better, you are trying to run it on something that is corrupted or invalid.