-
Notifications
You must be signed in to change notification settings - Fork 23
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 lines table overflow detection segfault #101
Fix lines table overflow detection segfault #101
Conversation
Could you write a few lines to explain what this patch does? And where the values |
I think the title explains it pretty well. You're currently reading unmapped memory to find where the lines table ends, and (wrongly) expecting zeroes when you do (instead of garbage or an outright crash). The values have to do with AMX structure. I honestly do not remember. This is tested, working, and most importantly correct (at least more than the original code...). |
OK, that's great, thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Cheaterman Thanks for submitting this PR. Please check my comments if you still want to have this merged.
); | ||
unsigned int lines_overflow_count = 0; | ||
|
||
while(biggest_possible_size < dbghdr.size) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to convert the byte order via amx_AlignXXX()
calls like in the original AMX implementation to preserve support for Big Endian systems. Who knows what happens in the future, maybe someone ends up using this on ARM. So I'd like this to stick to the existing conventions that the rest of the AMX implementation adheres to.
@@ -123,14 +123,14 @@ typedef struct tagAMX_DBG_STATE { | |||
} AMX_DBG_STATE PACKED; | |||
|
|||
typedef struct tagAMX_DBG { | |||
AMX_DBG_HDR _FAR *hdr PACKED; /* points to the AMX_DBG header */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These whitespace changes are not relevant to the patch. Please revert.
} /* while */ | ||
unsigned int biggest_possible_size = ( | ||
sizeof(AMX_DBG_HDR) | ||
+ dbghdr.files * (sizeof(AMX_DBG_FILE) + 33) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not clear what 33 stands for. I for one don't remember what it means. Can you put this in a constant (pre-processor #define to be exact) or infer it from some sizeof
arithmetic it it corresponds to the size of some structures?
|
||
ptr += ( | ||
dbghdr.lines * sizeof(AMX_DBG_LINE) | ||
+ lines_overflow_count * 0x10000 * sizeof(AMX_DBG_LINE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use a named constant like (IN16_MAX+1) instead of 0x10000
#endif | ||
ptr += ((uint32_t)dbghdr.lines + 1) * sizeof(AMX_DBG_LINE); | ||
} /* while */ | ||
unsigned int biggest_possible_size = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The source code of AMX is written in ANSI C, which does not allow declaring variables after statements. Let's stick to that convention to not introduce compilation issues and for consistency sake.
Ended up implementing it a little differently in master. |
No description provided.