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

Allow *input script command to support negative input #2375

Merged
merged 3 commits into from Jun 1, 2019

Conversation

AnnieRuru
Copy link
Contributor

Pull Request Prelude

Issues addressed

prontera,155,185,5	script	kljhdsfskjh	1_F_MARIA,{
	.@b = input(.@a, -1000,100);
	dispbottom .@a +" "+ .@b;
	end;
}

input -100, always gets 0

Changes Proposed

blame clif.c, now allow negative input

Affected Branches

  • Master

Known Issues and TODO List

rathena also allow it, why hercules don't ?

@HerculesWSAPI
Copy link
Contributor

This change is Reviewable

@4144
Copy link
Contributor

4144 commented Feb 19, 2019

for avoid exploits, probably need check limit inside clif_parse_NpcAmountInput
because by default negative values may create some issues

@AnnieRuru
Copy link
Contributor Author

yeah, no idea who so clever wants to avoid it in the source code
because input_min_value was commented out ... thanks for pointing it out

@AnnieRuru AnnieRuru added type:bug Issue is a bug or describes an incorrect behavior that should be fixed status:code-review Awaiting code review component:core:scriptengine Affecting the script engine or the script commands and removed status:code-review Awaiting code review labels Feb 28, 2019
@MishimaHaruna MishimaHaruna added this to the Release v2019.04.07 milestone Apr 7, 2019
@4144
Copy link
Contributor

4144 commented Apr 7, 2019

@MishimaHaruna from what i see it can be unsafe as is. because it allow negative value stored in npc variable and after other code will use it as it

@MishimaHaruna
Copy link
Member

But as far as I'm aware it's already possible to store negative values in NPC variables (through direct assignment), am I missing anything?

The default setting that this PR uses, provides the same behavior as before, in scripts that used to rely on input() using a minimum limit of 0 when not otherwise specified (see BUILDIN(input)). This will only affect scripts that specifically request a different minimum limit, or servers that alter the default configuration

@4144
Copy link
Contributor

4144 commented Apr 7, 2019

For now npc_amount used only in BUILDIN(input) and here it checked, but if some other code will use npc_amount it can be negative. I suggest add validation inside clif_parse_NpcAmountInput after this all other code will be safe in same way as before current pr commit

@4144
Copy link
Contributor

4144 commented Apr 7, 2019

better not allow unsafe values at first place and not check it every time before it can be used

@MishimaHaruna
Copy link
Member

Oh I see, this is a good point. Let's move this to the next release then.

@AnnieRuru
Copy link
Contributor Author

I'm sorry, I'm too dump to understand your point
can you provide a patch to prove your point ?
this should be pretty simple and straight forward, I guess

@4144
Copy link
Contributor

4144 commented Apr 10, 2019

it short into sd need add also variables npc_amount_min and npc_amount_max
this vars should be set in buildin input before actual input happend. Next input value should be checked in clif_parse_NpcAmountInput for this new limits. Current check inside input buildin can stand as is

sd->npc_amount_min and sd->npc_amount_max to limit input range
and sd->npc_input_capped_range because the documentation ask for it
@AnnieRuru
Copy link
Contributor Author

you mean like this ?
so its not adding 2 variables, it should be 3 variables because the documentation ask for it

The command has two optional arguments and a return value.
The default value of 'min' and 'max' can be set with 'input_min_value' and
'input_max_value' in conf/map/script.conf.
For numeric inputs the value is capped to the range [min, max]. Returns 1
if the value was higher than 'max', -1 if lower than 'min' and 0 otherwise.
For string inputs it returns 1 if the string was longer than 'max', -1 is
shorter than 'min' and 0 otherwise.

in fact, I always do checks like this in my npc script
https://github.com/AnnieRuru/Release/blob/master/scripts/Games%20%26%20Events/Guess%20the%20password/hangman_0.1.txt#L57-L60

@4144
Copy link
Contributor

4144 commented Apr 10, 2019

variables optional only for scripts, inside buildin you always have limits from parameters or some default one. default i think 0 to INT_MAX

@AnnieRuru
Copy link
Contributor Author

AnnieRuru commented Apr 10, 2019

I'm kinda give up, you can take over
because I totally don't understand what you are talking about

the 0 to INT_MAX has been double check by input_min_value and input_max_value
or maybe I misunderstood your meaning

@MishimaHaruna MishimaHaruna merged commit 50a0952 into HerculesWS:master Jun 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:core:scriptengine Affecting the script engine or the script commands type:bug Issue is a bug or describes an incorrect behavior that should be fixed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants