alliedmodders / amxmodx

AMX Mod X - Half-Life 1 Scripting and Administration
http://www.amxmodx.org/
499 stars 203 forks source link

pc dbg size calculation is wrong #922

Closed afwn90cj93201nixr2e1re closed 3 years ago

afwn90cj93201nixr2e1re commented 3 years ago

Later...

afwn90cj93201nixr2e1re commented 3 years ago

https://github.com/alliedmodders/amxmodx/blob/master/compiler/libpc300/sc6.c#L1023-L1026

So, the main issues that generated wrong debug.size is right here.

It is produce the debug section size larger than it actually is. As a result, we have corrupted files, in fact, amxmodx doesn't really care if the file is valid or not, due to the fact that David rewrote reading the debug info from memory (instead file), so this (https://github.com/alliedmodders/amxmodx/blob/master/amxmodx/amxdbg.cpp#L139) is the only place in amxmodx where we can hypothetically get garbage instead of actual information, but since we continue to use only actual values from structure fields this problem doesn't show itself.

  /* symbol table */
  for (index = 0; (str = get_dbgstring(index)) != NULL; index++) {
      assert(str != NULL);
      assert(str[0] != '\0' && str[1] == ':');
      if (str[0] == 'S') {
          dbghdr.symbols++;
          str = strchr(str + 2, ':');
          assert(str != NULL);
          name = skipwhitespace(str + 1);
          str = strchr(name, ' ');
          assert(str != NULL);
          assert((int)(str - name) < sizeof symname);
          strlcpy(symname, name, (int)(str - name) + 1);
          dbghdr.size += (int32_t)(sizeof(AMX_DBG_SYMBOL) + strlen(symname));
          if ((prevstr = strchr(name, '[')) != NULL)
              while ((prevstr = strchr(prevstr + 1, ':')) != NULL)
                  dbghdr.size += sizeof(AMX_DBG_SYMDIM);
      } /* if */
  } /* for */

or

  /* symbol table */
  for (index=0; (str=get_dbgstring(index))!=NULL; index++) {
    assert(str!=NULL);
    assert(str[0]!='\0' && str[1]==':');
    if (str[0]=='S') {
      dbghdr.symbols++;
      name=skipwhitespace(strchr(str+2,':')+1);
      assert(name!=NULL);
      str=strchr(name,' ');
      assert(str!=NULL);
      assert((int)(str-name)<arraysize(symname));
      dbghdr.size+=sizeof(AMX_DBG_SYMBOL)+(str-name);
      if ((prevstr=strchr(name,'['))!=NULL)
        while ((prevstr=strchr(prevstr+1,':'))!=NULL) {
          dbghdr.size+=sizeof(AMX_DBG_SYMDIM);
        }
    }/* if */
  } /* for */

For example:

before fixes: file size = 7152 hea 3648 dbg size = 4077 (4055 (22)), hdr pos. (22) 3648, real. pos. 3670. so, 7152 - 3648 = 3504, 4077 + 3648 = 7725, but as we could see debug size is much more than we should read.

after fixes: file size = 7152 hea 3648 dbg size = 3504 (3482 (22)), hdr pos. (22) 3648, real. pos 3670. so, 7152 - 3648 = 3504, 3648 + 3504 = 7152.

I mean probably you could refactor the whole function from actual sources, the most places are about refactoring here, excluding tag definition's and stuff like that.

So, if we gonna add checks directly to amxmodx, i mean inside module, then we can make the old plugins stop working, but as we already know that garbage information doesn't bother us much (but maybe we could try to access the protected memory page and get crashes?), probably it is not needed and ... i mean... u know what im talking about, but still, due to the fact that we read strictly only the fields, we do not refer to it, probably we should validate file size, no?

afwn90cj93201nixr2e1re commented 3 years ago

@dvander

thoughts?

Could you add fix by ur self?