Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix integer overflow. #62

Closed
wants to merge 1 commit into from
Closed

Fix integer overflow. #62

wants to merge 1 commit into from

Conversation

Rot127
Copy link
Member

@Rot127 Rot127 commented Nov 15, 2023

Some demangling let n grow above its max positive int. For these cases int is promoted to int64 and demangle_class_name() always returns unsuccessful if n < 0.

Some demangling let n grow above its max positive int.
For these cases int is promoted to int64 and demangle_class_name()
always returns unsuccessful if n < 0.
@karliss
Copy link
Member

karliss commented Nov 16, 2023

You just added third broken oveflow detection attempt on top of two previous broken ones. All of that is layers of bad fixes attempting to detect overflow and fighting the compiler instead of fixing the actual problem.

I suggest replacing it with something like this (use whatever equivalent of INT_MAX is suitable in the context of this library)

int count = 0;

if (!isdigit((unsigned char)**type))
     return -1;

while (isdigit((unsigned char)**type)) {
    if (count > (INT_MAX - 9)/10) {
        return -1;
    }
    count *= 10;
    count += **type - '0';
    (*type)++;
}

return count;

No volatiles, no complicated reasoning about what happens to divisibility by 10 after overflow, no int64 or additional casts.

For proper general purpose integer parser that needs to work correctly for every possible int value up to maxium, code needs to be little more complicate. (assuming 32 bit integers) his check will have false positive thus rejecting numbers 2147483640- 2147483647.
But for consume_count which if I am not mistaken used only for counts of bytes and entries within the mangled names, you are unlikely to have anything close to 2GB mangled name.

C is isn't assembly any attempts of trying to detect signed integer overflow after the fact is fools errand, sooner or later compiler will decide that you are attempting to summon nasal demons.

Only reliable ways of dealing with it is:

  • if wrapping behavior is desired use unsigned integers where appropriate (mostly for cryptography, compression and similar algorithms)
  • do the check before the potentially overflowing operation, the mere fact that operation overflows is the problem, not the code trying to reason about it afterwards
  • use appropriate compiler intrinsics for operation+overflow check at the same time like __builtin_smul_overflow, downside that it requires complier specific code which is annoying if you want to support many different platforms and compilers.

@wargio
Copy link
Member

wargio commented Nov 16, 2023

the issue is that code is full of bugs & vulns due its age. it would make more sense to just rewrite it

@Rot127
Copy link
Member Author

Rot127 commented Nov 16, 2023

Sorry, I should have explained why I made this fix. It is really just a dirty one because libdemangle segfaulted for one of my binaries due to this.
@karliss Your way would definitely be the better fix. Though I had similar thinking as @wargio. We should just replace the whole thing. It is full of bugs. Literally just: look and choose what you want to break. This is why I not even attempted to do it properly.

@wargio
Copy link
Member

wargio commented Nov 17, 2023

Sorry, I should have explained why I made this fix. It is really just a dirty one because libdemangle segfaulted for one of my binaries due to this. @karliss Your way would definitely be the better fix. Though I had similar thinking as @wargio. We should just replace the whole thing. It is full of bugs. Literally just: look and choose what you want to break. This is why I not even attempted to do it properly.

there is an issue only for this. this has to be rewritten to remove this GPL code and have LGPL one

@wargio
Copy link
Member

wargio commented Apr 1, 2024

Fixed in 498e007

@wargio wargio closed this Apr 1, 2024
@wargio wargio deleted the int-overflow branch April 1, 2024 02:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants