Unit tests on byte extraction might have a wrong expected result

[xusheng6]

Reading the code at https://github.com/mandiant/capa/blob/master/tests/fixtures.py#L646-L649, I am wondering why the unit test would be expecting to extract those utf16-le strings as bytes sequence rather than strings? Because this comment says we should NOT extract obvious strings as bytes:

https://github.com/mandiant/capa/blob/master/capa/features/extractors/ida/insn.py#L175-L176

Yet, these are clearly "obvious" utf16-le strings:

I feel like:

1. The unit test itself is problematic, and the expected test result should be False.
2. We can add a unit test for string extraction, and then the expected result should be True.
3. The current unit test passes for IDA extractor because there is a bug in the way extract_insn_bytes_features is written, see this issue.
4. I am not sure why the viv extractor would behave in the same way, there might be a problem with isProbablyString, but I have not checked its doc or implementation.

[xusheng6]

I am filing this because the binja extractor is failing on these tests, but I think its behavior is correct.

[mr-tz]

Good catch. I did not update the tests as part of the change in #1298.

1. Yes
2. Yes
3. Good find
4. I'll take a look

If you want, would be great if you can submit the respective fixes (at least for 1-3).

[xusheng6]

Sure, I will submit a PR soon.

However, it will be rejected by the CI until vivisect is fixed as well. I guess you can work on top of my upcoming PR.

[mr-tz]

Thanks! I'm looking into it now.

[xusheng6]

I force pushed moments ago (to fix my fat finger). You will wish to pull it again to sync.

…

On Wed, Mar 1, 2023 at 17:54 Moritz ***@***.***> wrote: Thanks! I'm looking into it now. — Reply to this email directly, view it on GitHub <#1336 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AWRACE2O7Y3GEUJHGVYCD6LWZ4MDZANCNFSM6AAAAAAVLY5BTI> . You are receiving this because you authored the thread.Message ID: ***@***.***>