Bug 4061 - Bug in implementation of native function read_flags.
Bug in implementation of native function read_flags.
Status: RESOLVED WONTFIX
Product: AMX Mod X
Classification: Unclassified
Component: Core
trunk
PC All
: P3 normal
Assigned To: amxmodx-bugs@alliedmods.net
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-10-22 06:14 PDT by Lev2001
Modified: 2013-06-16 01:49 PDT (History)
3 users (show)

See Also:


Attachments
Proposed patch (115 bytes, patch)
2009-10-22 06:14 PDT, Lev2001
dvander: review? (hv.contact)
hv.contact: feedback-
Details | Diff | Splinter Review

Description Lev2001 2009-10-22 06:14:57 PDT
Created attachment 1758 [details] [review]
Proposed patch

By default meaning "flag" is a letter from 'a' to 'z', but function read_flags tries to convert any char from 1 to 255 into flag.
Proposed patch adds check for char is in range from 'a' to 'z'.
Please check it because i didn't tried to compile with it.
Comment 1 David Anderson [:dvander] 2009-10-22 14:33:41 PDT
The allowed input range is undefined, this patch would make it explicit. Probably harmless, I suspect both that:
 1. this bug doesn't really effect anyone
 2. the patch would not regress compatibility
Comment 2 Lev2001 2009-10-22 21:01:50 PDT
I didn't understand the conclusion: will you confirm it or not?
Also have a look at get_flags function implementation - it converts int to chars flags and it checks only for 0 to 25 bits (i.e. 'a' to 'z').
So may be read_flags should be opposite?
Because now you can send "!$614^!46" to read_flags and it will return some int value.
If then you will pass that value to get_flags you will receive some flags set.
Comment 3 David Anderson [:dvander] 2009-10-22 21:04:32 PDT
reread comment #1

read_flags does not specify valid inputs or outputs. so technically the functionality could be seen as legal, and abused or relied on in some way by plugins. on the other hand, it's unlikely.

for both of these reasons i think it's okay to take the patch, and that the bug is very minor. but i still want to verify the potential compatibility impact.
Comment 4 David Anderson [:dvander] 2013-02-13 00:57:48 PST
Comment on attachment 1758 [details] [review]
Proposed patch

Given the compatibility risk, I'll let this sit for 1.8.2. Flipping review? to arkshine.
Comment 5 Vincent Herbet [:Arkshine] 2013-02-14 06:13:04 PST
Comment on attachment 1758 [details] [review]
Proposed patch

1) The patch is wrong, it may cause problems (i.e: infinite loop if character is not between 'a' and 'z' since *c would not be increased)
2) The patch will broke non-conventional usage of the native, even if such usage is unlikely, there is still a potential risk considering how the native is often used.

I would conclude the native doesn't require fixing.
It would be safe to keep the actual behavior.

Note You need to log in before you can comment on or make changes to this bug.