Hacker News new | ask | show | jobs
by boutique 1157 days ago
Thanks a lot for the guidance/tip, I've learned something new. And you're absolutely right about the cause of the mentioned crash -- I've updated the Github issue with a bit of new info I've gathered.

Regarding the function, here it is: https://github.com/dreamstalker/rehlds/blob/master/rehlds/fi...

Interestingly, strdup gets compiled into:

  89 04 24           mov   [esp+101Ch+name], pszContentPath ; s
  E8 82 DC 00 00     call  strlen
  66 C7 04 03 2F 00  mov   word ptr [pszContentPath+eax], 2Fh ; '/'
Which is basically:

  *(_WORD *)&pPath[strlen(pPath)] = '/';`
and would explain why Valgrind says it goes one byte over.
1 comments

Yeah, looks like the Q_strcat(pszContentPath, "/"); is invalid, as glibc has only allocated exactly enough to fit the path in the buffer returned by realpath().

The compiler seems to completely inline the strcat and write the '/' and null as a single 2-byte word write, the null then being out of bounds of the malloc'd chunk and likely causing the error as it overwrites something important.

Interestingly, the open group spec says that a null argument to realpath is "Implementation defined" [0]

And the linux (glibc) man pages say it allocates a buffer "Up to PATH_MAX" [1]

I guess "strlen(path)" is "Up to PATH_MAX", but the man page seems unclear - you could read that as implying the buffer is always allocated to PATH_MAX size, but that's not what seems to be happening, just effectively calling strdup() [2]. I have no idea how to feed back to the linux man pages, but might be worth clarifying there.

[0] https://pubs.opengroup.org/onlinepubs/009696799/functions/re...

[1] https://linux.die.net/man/3/realpath

[2] https://github.com/bminor/glibc/blob/0b9d2d4a76508fdcbd9f421...