PE: Assume AddressOfIndex could not be in raw section, check callback is zero right after it reads from binary

[kkent030315]

• Fixed issue Fail to parse PE files compiled by Rust #424 where binaries compiled Rust, and binaries with AddressOfIndex only exists in virtual address caused Malformed error by simply not raising explicit error but set the slot field to None.
• Fixed similar issue where the callback terminator check must be right before it reads from the raw binary. Otherwise it overflows when subtracting by image base.

[kkent030315]

I think we can safely remove slot field as an alternative option. As explained in the issue, addres of indexes field is only useful at runtime and I personally think that field should not exists in the goblin while it is always useless for static binary analysis.

@m4b How about removing the slot field and its parsing logic instead? (perhaps subject to breaking change if this option applies)

[m4b]

I personally think that field should not exists in the goblin while it is always useless for static binary analysis.

I'm confused, didn't you just add this field in 6c5b3e8 ?

I will merge this patch, thanks for posting it so quickly!

Re removing the slot field, I'd really like to avoid churn as much as possible, specifically in context of breaking changes. I know we can't predict the future but for future patches, try considering how breaking change could be avoided, or if a field/function really needs to be pub, etc.

While we're talking about that, I will likely deprecate the parse and parse_with_opt for the TlsData struct; I should have caught this in review, that's on me, but the use of generics there to get the u32/u64 is unintuitive, and is only used effectively to do mem::std::size_of::<T>() == 8; ideally the is_64 bit should have been used, or I have a patch that uses scroll to parse this generically, but it's very ugly, and I wouldn't want it to be pub anyway.

I think a more goblin-idiomatic way to do it (on hindsight) would have been to write:

ImageTlsDirectory
ImageTlsDirectory32

and then write:

impl From<ImageTlsDirectory32> for ImageTlsDirectory {}

and then in the parse mechanism, if we're passed e.g., is_64 or whatever, we simply do:

let tls_directory = if is_64 { parse::<ImageTlsDirectory>() } else { parse::<ImageTlsDirectory32>().into() }

this alas will be a breaking change since the parse uses generics right now; anyway something to fix in the 0.10 release, and it's fairly minor.

anyway thanks for this fix!

[kkent030315]

I personally think that field should not exists in the goblin while it is always useless for static binary analysis.

I'm confused, didn't you just add this field in 6c5b3e8 ?

Yes I did. Some binaries has AddressOfIndex valid in raw data (unlike Rust binaries in question) but nothing really useful. I just noticed it now😅.

[gabrielesvelto]

I guess this fixed #411 too?

[kkent030315]

I guess this fixed #411 too?

Apparently it seems. Parsing nvml.dll won't raise any errors now.

cargo run --example rdr -- nvml.dll