Skip to content

Storage item list packet size fix and minor clif.c fixes #2424

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 3 commits into from
May 5, 2019
Merged

Storage item list packet size fix and minor clif.c fixes #2424

MishimaHaruna merged 3 commits into from
May 5, 2019

Conversation

@hemagx
Copy link
Contributor

@hemagx hemagx commented Apr 8, 2019

Pull Request Prelude

Changes Proposed

  1. Fixed a packet size underflow in storage list sending which started with client supporting int32 for item id and added static assert to detect this problem if ever happened again.
  2. Fixed direct calls to clif_send in clif.c
  3. Add missing nullpo and assert to clif_send

Issues addressed:
None

@HerculesWSAPI
Copy link
Contributor

HerculesWSAPI commented Apr 8, 2019

This change is Reviewable

@hemagx hemagx requested a review from MishimaHaruna April 8, 2019 02:58
@hemagx hemagx changed the title Hercules packet fixes Storage item list packet size fix and minor clif.c fixes Apr 8, 2019
4144
4144 requested changes Apr 8, 2019
View reviewed changes
src/map/clif.c Outdated
int i = 0;
struct item_data *id;

#define STORAGE_ITEMS_MAX_ITERATION 500
Copy link
Contributor

@4144 4144 Apr 8, 2019

Choose a reason for hiding this comment

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

probably better on the fly calculate max possible items per packet for current packet version?
and move this define to one of headers?

Copy link
Contributor Author

@hemagx hemagx Apr 8, 2019

Choose a reason for hiding this comment

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

That would be better to calculate for each packer version indeed, the reason i did not put the define in header is because i want to keep the scope of the define for this function only.

Copy link
Contributor Author

@hemagx hemagx Apr 8, 2019

Choose a reason for hiding this comment

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

I will rewrite the function to be more flexible.

@hemagx
Copy link
Contributor Author

hemagx commented Apr 8, 2019

@4144 Please check the new code, i have not tested it yet, but i will test it and rebase the branch tomorrow after my work shift ~

4144
4144 approved these changes Apr 8, 2019
View reviewed changes
Copy link
Contributor

@4144 4144 left a comment

Choose a reason for hiding this comment

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

can you also fix storage names?
Current code strip last byte from name
need use name len: NAME_LENGTH + 1

@hemagx
Copy link
Contributor Author

hemagx commented Apr 9, 2019

can you also fix storage names?
Current code strip last byte from name
need use name len: NAME_LENGTH + 1

Is not NAME_LENGTH = (23 + 1), it should be enough to use it? no? or the client doesn't require null-terminator anymore?

@4144
Copy link
Contributor

4144 commented Apr 9, 2019

ah sorry, issue was in ZC_INVENTORY_START, but i thinked in clif_storageItems

@4144
Copy link
Contributor

4144 commented Apr 9, 2019

i will fix ZC_INVENTORY_START by self

@hemagx
Copy link
Contributor Author

hemagx commented Apr 9, 2019

Okay splendid!

hemagx added 2 commits April 10, 2019 16:25
@hemagx
Rewrite clif_storageItems
37767f6 
- The maximum packetsize is now decided during compile time depending on client version which fixes an issue started with clients supporting int32 as itemid where packet size would underflow
- The function now have a single loop that is easier to read and understand

Signed-off-by: Ibrahim Zidan <[email protected]>
@hemagx
Addition of missing nullpo check in clif_send and an assertion check for
5410de9 
zero/negatvie length values

Signed-off-by: Ibrahim Zidan <[email protected]>
@hemagx hemagx force-pushed the hercules_packet_fixes branch from 2fcb173 to 5c0cdbb Compare April 10, 2019 14:25
@hemagx
Copy link
Contributor Author

hemagx commented Apr 10, 2019

@4144 Fixed some minor issues and rebased, please check he new code.

4144
4144 requested changes Apr 10, 2019
View reviewed changes
@hemagx hemagx force-pushed the hercules_packet_fixes branch from 5c0cdbb to 112b19d Compare April 10, 2019 15:28
@hemagx hemagx requested a review from 4144 April 10, 2019 15:51
@hemagx hemagx added this to the Release v2019.05.05 milestone Apr 12, 2019
4144
4144 requested changes Apr 17, 2019
View reviewed changes
@hemagx hemagx requested a review from 4144 April 25, 2019 14:31
@MishimaHaruna MishimaHaruna merged commit 416acb0 into HerculesWS:master May 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@4144 4144 4144 approved these changes

@dastgirp dastgirp dastgirp approved these changes

@MishimaHaruna MishimaHaruna Awaiting requested review from MishimaHaruna

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

Release v2019.05.05

Development

Successfully merging this pull request may close these issues.

5 participants

@hemagx @HerculesWSAPI @4144 @dastgirp @MishimaHaruna