Vermin assumes byte union to be a type union

[Wenzel]

Describe the bug

The following line is assumed to be a type union (3.10) by vermin:

if (global_byte | local_byte) != global_byte:

To Reproduce
Found while scanning the kafl.fuzzer project with vermin.

$ vermin -vvv kafl_fuzzer | grep 3.10
!2, 3.10     /home/mtarral/kAFL/kafl/fuzzer/kafl_fuzzer/manager/bitmap.py
  L93: union types as `X | Y` requires !2, 3.10
Minimum required versions: 3.10

You can check the "faulty" line

Expected behavior
This shouldn't be flagged as a union type (and shouldn't require 3.10, since our software runs fine on 3.8)

Environment (please complete the following information):

• Vermin version: 1.4.2 (pip)

Thanks for vermin !

[netromdk]

Thanks for reporting this! I'll look into it soon.

[netromdk]

I took a swing at it and now it doesn't yield the false positive anymore:

% vermin -vvv kafl.fuzzer | grep 3.10 ; echo $?
1

With:

2.7, 3.0     kafl.fuzzer/kafl_fuzzer/manager/bitmap.py
  L11 C7: 'ctypes' module requires 2.5, 3.0
  L12 C7: 'inspect' module requires 2.1, 3.0
  L80: function decorators requires 2.4, 3.0
  L84 C15: 'all' member requires 2.5, 3.0
  L123: `"..{}..".format(..)` requires 2.7, 3.0
  L123: 'str.format' member requires 2.6, 3.0

I'm just ensuring things are as they should be and if I can think of other test cases.
But at least all old tests pass, too. 🤞🏻

[Wenzel]

HI @netromdk and thanks for the quick feedback.

I took a swing at it and now it doesn't yield the false positive anymore:

Do you mean that you looked at it, and you can't repro the issue ?
Or you already made a change and now it's working as expected.

Because I cloned the repo again, this time on Windows, and I can repro the issue:

Thanks !

[netromdk]

Hi! It's in a branch right now. I was able to reproduce and fix it.

You can try it out: issue-103

[netromdk]

Let me know if it fixed it for you or if you have any additional feedback. Thanks!

[Wenzel]

Yes, I confirm it works on my side !

Thank you very much 🙏

[netromdk]

Great! The fixes have been merged to master. You can use that for now. I'm in the middle of a larger release, unfortunately, so an official release will have to wait a little bit. Thanks again.

[gsingh93]

I was running into this issue as well, and this fixed most of the issues for me. But there's one left, it looks like this:

from math import *

print(int(pi) | int(pi))

Essentially if the variable is imported with a wildcard, it's treated as a type union. This case might be harder to fix because the variable being used is defined in another library, but I wanted to check if it's possible.

[netromdk]

As of 530c41f (master at the time of writing), that one is not reported as a type union, only:

  print(expr) requires 2.0, 3.0

[gsingh93]

Hmm, I tried to simplify my actual example but I guess I made a mistake somewhere. Here's the actual code that's causing a problem:

from capstone import *

print(CS_MODE_MCLASS | CS_MODE_THUMB)

It's using the capstone library.

I'm at 530c41f, and I'm getting this:

❯ ./vermin.py -vv ~/code/vermin-test.py
Detecting python files..
Analyzing using 16 processes..
!2, 3.10     /home/gsgx/code/vermin-test.py
  print(expr) requires 2.0, 3.0
  union types as `X | Y` require !2, 3.10

Tips:
- Since '# novm' or '# novermin' weren't used, a speedup can be achieved using: --no-parse-comments
(disable using: --no-tips)

Minimum required versions: 3.10
Incompatible versions:     2

[netromdk]

Yeah, in such a case Vermin cannot know for sure if it's a type or something else, unfortunately.

But you can make Vermin ignore the line via:

print(CS_MODE_MCLASS | CS_MODE_THUMB)  # novm

Or using # novermin.

[disconnect3d]

Another case like this:

from mmap import mmap, MAP_ANON, MAP_PRIVATE, PROT_EXEC, PROT_READ, PROT_WRITE
mmap(-1, 1024 * 1024 * 1024, MAP_PRIVATE | MAP_ANON, PROT_WRITE | PROT_READ | PROT_EXEC)

$ vermin -vvvv -t=3.6 --violations x.py
Detecting python files..
Analyzing using 16 processes..
!2, 3.10     /Users/dc/projects/pwndbg/x.py
  L2: union types as `X | Y` require !2, 3.10

Wouldn't it be better to validate union types only in places where you can usually find type annotations?

An alternative could be 1) tracking imports and 2) trying to evaluate the constants. This may be risky about any side effects from those imports though.

[netromdk]

Thanks. I'll revisit this.

It's an interesting thought to validate especially when type annotations are in place. But that doesn't cover all possibilities, unfortunately. Vermin already tracks imports. Evaluating constants is not going to be a valid approach since Vermin parses the abstract syntax tree but doesn't actually know all underlying details. This is why heuristics are used in some cases.

If there are more false positives in general, we might also consider making these checks opt-in instead with caveats.

[szabi]

Wouldn't it be better to validate union types only in places where you can usually find type annotations?

Unfortunately, union types can be used as (syntactically) function parameters, so mmap(-1, 1024 * 1024 * 1024, MAP_PRIVATE | MAP_ANON, PROT_WRITE | PROT_READ | PROT_EXEC) would still pass:

Union types can be used in issubclass(...) and isinstance(...) queries!

[netromdk]

Due to all of the false positives and the need for heuristics, I've chosen to make union types detection an opt-in feature via --feature union-types (#176).