Skip to content

Fixed a condition in RoDEX attachment #2266

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.

Sign up for GitHub

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

Jump to bottom
Merged
MishimaHaruna merged 1 commit into from
Jan 12, 2020
Merged

Fixed a condition in RoDEX attachment #2266

MishimaHaruna merged 1 commit into from
Jan 12, 2020

Conversation

@dastgirp
Copy link
Member

@dastgirp dastgirp commented Oct 7, 2018

Pull Request Prelude

Changes Proposed

The condition if( sd->mail.inbox.msg[i].zeny + sd->status.zeny > MAX_ZENY ) { would overflow and turn into negative value, resulting in the code never executing even if it exceed MAX_ZENY

Affected Branches:

Issues addressed:

Known Issues and TODO List

@HerculesWSAPI
Copy link
Contributor

HerculesWSAPI commented Oct 7, 2018

This change is Reviewable

@HerculesWSAPI
Copy link
Contributor

HerculesWSAPI commented Oct 7, 2018

This pull request fixes 1 alert when merging 64bbdb5 into 852c133 - view on LGTM.com

fixed alerts:

  • 1 for Comparison result is always the same

Comment posted by LGTM.com

@dastgirp dastgirp changed the base branch from stable to master October 7, 2018 16:35
@HerculesWSAPI
Copy link
Contributor

HerculesWSAPI commented Oct 7, 2018

This pull request fixes 1 alert when merging 64bbdb5 into 7120569 - view on LGTM.com

fixed alerts:

  • 1 for Comparison result is always the same

Comment posted by LGTM.com

@dastgirp dastgirp requested review from 4144 and Helianthella October 13, 2018 11:05
@dastgirp dastgirp added component:core Affecting the Hercules core (i.e. not the game mechanics directly) status:code-review Awaiting code review severity:2-fair Incorrect gaming mechanics, database warnings labels Oct 13, 2018
@HerculesWSAPI
Copy link
Contributor

HerculesWSAPI commented Oct 13, 2018

This pull request fixes 1 alert when merging 64bbdb5 into baeb7a1 - view on LGTM.com

fixed alerts:

  • 1 for Comparison result is always the same

Comment posted by LGTM.com

4144
4144 requested changes Oct 13, 2018
View reviewed changes
src/map/clif.c
return;

if( sd->mail.inbox.msg[i].zeny + sd->status.zeny > MAX_ZENY ) {
if( sd->mail.inbox.msg[i].zeny > MAX_ZENY - sd->status.zeny ) {
Copy link
Contributor

@4144 4144 Oct 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for correct check need add this
if (sd->status.zeny >= MAX_ZENY || sd->mail.inbox.msg[i].zeny > MAX_ZENY - sd->status.zeny) {
because if change MAX_ZENY from INT_MAX to some small number can be issue

Copy link
Member Author

@dastgirp dastgirp Oct 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can sd->status.zeny be > MAX_ZENY at any moment?
It can be MAX_ZENY, but that would result in 0 in second condition, which would not cause any issue

@MishimaHaruna MishimaHaruna added this to the Release v2020.01.12 milestone Jan 12, 2020
@MishimaHaruna MishimaHaruna merged commit 776befc into HerculesWS:master Jan 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@4144 4144 4144 requested changes

@MishimaHaruna MishimaHaruna MishimaHaruna approved these changes

@Helianthella Helianthella Awaiting requested review from Helianthella

Assignees

No one assigned

Labels

component:core Affecting the Hercules core (i.e. not the game mechanics directly) severity:2-fair Incorrect gaming mechanics, database warnings status:code-review Awaiting code review

Projects

None yet

Milestone

Release v2020.01.12

Development

Successfully merging this pull request may close these issues.

4 participants

@dastgirp @HerculesWSAPI @4144 @MishimaHaruna