Question MAX_NAME_LENGTH crash, repeatable, all versions

Castillo wrote: Maybe, your version of the library is diffrent, and doesn't detect it... and, since it's a small overflow it's doesn't affect the program execution on your system maybe?

Doubt it. OpenBSD's philosophy is that overflows because there's no room for NULL should crash and crash hard. Either the compiler is swapping strlen() for another function, and from my understanding that only occurs when it runs across strncat() or strncpy(), or there's a bug.

I did some tests:

gcc test.c
./a.out

that is ok.

BUT, if i compile using the same gcc parameters as TBA mud. i get this:

rescator@rescator-System-Product-Name:~$ gcc -g -O2 -Wall -Wno-char-subscripts -Wno-unused-but-set-variable test.c
In file included from /usr/include/string.h:635:0,
from test.c:2:
In function ‘strcpy’,
inlined from ‘main’ at test.c:11:2:
/usr/include/x86_64-linux-gnu/bits/string3.h:110:10: warning: call to __builtin___memcpy_chk will always overflow destination buffer
return __builtin___strcpy_chk (__dest, __src, __bos (__dest));
^
rescator@rescator-System-Product-Name:~$ ./a.out
*** buffer overflow detected ***: ./a.out terminated
======= Backtrace: =========
/lib/x86_64-linux-gnu/libc.so.6(+0x777e5)[0x7f77d3e6a7e5]
/lib/x86_64-linux-gnu/libc.so.6(__fortify_fail+0x5c)[0x7f77d3f0c15c]
/lib/x86_64-linux-gnu/libc.so.6(+0x117160)[0x7f77d3f0a160]
./a.out[0x4004eb]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0)[0x7f77d3e13830]
./a.out[0x400539]
======= Memory map: ========
00400000-00401000 r-xp 00000000 08:01 5505307 /home/rescator/a.out
00600000-00601000 r--p 00000000 08:01 5505307 /home/rescator/a.out
00601000-00602000 rw-p 00001000 08:01 5505307 /home/rescator/a.out
0066c000-0068d000 rw-p 00000000 00:00 0 [heap]
7f77d3bdd000-7f77d3bf3000 r-xp 00000000 08:01 5640757 /lib/x86_64-linux-gnu/libgcc_s.so.1
7f77d3bf3000-7f77d3df2000 ---p 00016000 08:01 5640757 /lib/x86_64-linux-gnu/libgcc_s.so.1
7f77d3df2000-7f77d3df3000 rw-p 00015000 08:01 5640757 /lib/x86_64-linux-gnu/libgcc_s.so.1
7f77d3df3000-7f77d3fb3000 r-xp 00000000 08:01 5636313 /lib/x86_64-linux-gnu/libc-2.23.so
7f77d3fb3000-7f77d41b3000 ---p 001c0000 08:01 5636313 /lib/x86_64-linux-gnu/libc-2.23.so
7f77d41b3000-7f77d41b7000 r--p 001c0000 08:01 5636313 /lib/x86_64-linux-gnu/libc-2.23.so
7f77d41b7000-7f77d41b9000 rw-p 001c4000 08:01 5636313 /lib/x86_64-linux-gnu/libc-2.23.so
7f77d41b9000-7f77d41bd000 rw-p 00000000 00:00 0
7f77d41bd000-7f77d41e3000 r-xp 00000000 08:01 5636299 /lib/x86_64-linux-gnu/ld-2.23.so
7f77d43c0000-7f77d43c3000 rw-p 00000000 00:00 0
7f77d43e1000-7f77d43e2000 rw-p 00000000 00:00 0
7f77d43e2000-7f77d43e3000 r--p 00025000 08:01 5636299 /lib/x86_64-linux-gnu/ld-2.23.so
7f77d43e3000-7f77d43e4000 rw-p 00026000 08:01 5636299 /lib/x86_64-linux-gnu/ld-2.23.so
7f77d43e4000-7f77d43e5000 rw-p 00000000 00:00 0
7fffb41e9000-7fffb420a000 rw-p 00000000 00:00 0 [stack]
7fffb42bb000-7fffb42bd000 r--p 00000000 00:00 0 [vvar]
7fffb42bd000-7fffb42bf000 r-xp 00000000 00:00 0 [vdso]
ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0 [vsyscall]
Aborted (core dumped)

But, this code: With the +1
rescator@rescator-System-Product-Name:~$ gcc -g -O2 -Wall -Wno-char-subscripts -Wno-unused-but-set-variable test.c
rescator@rescator-System-Product-Name:~$ ./a.out

No warnings, no crash.

Interesting.
Do you understand that?

My solution was simply to change the integer comparison in interpreter.c from A > B to A >= B as follows:
to
If I remember my first year CS classes correctly, the array of type char is zero indexed with a null char terminator.

With MAX_ARRAY+1, we're doing a simple A > B integer comparison.
With MAX_ARRAY, we're doing a simple A >= B integer comparison.
Isn't the effect essentially the same?

Some instances of [MAX_NAME_LENGTH +1] exist in the code already such as:
github.com/tbamud/tbamud/blob/7f0acefcb4...rc/spec_procs.c#L583
github.com/tbamud/tbamud/blob/7f0acefcb4...7ea/src/house.c#L295

Yet there are other instances of [MAX_NAME_LENGTH] in some admin functions such as:
github.com/tbamud/tbamud/blob/7f0acefcb4...c/act.wizard.c#L4638

But most interestingly is the implementation of [MAX_NAME_LENGTH] in ban.c
github.com/tbamud/tbamud/blob/7f0acefcb4...f007ea/src/ban.c#L41

In ban.c the author uses strncpy() vs strcpy() to only copy n=MAX_NAME_LENGTH characters to the string which is size MAX_NAME_LENGTH+1.

So I guess what I'm getting at is this: What is the BEST, most portable, solution?

With MAX_ARRAY+1, we're doing a simple A > B integer comparison.
With MAX_ARRAY, we're doing a simple A >= B integer comparison.
Isn't the effect essentially the same?

The difference with >= will let you register 19 chars name instead of 20.
While MAX_ARRAY+1 seems more logical with the rest of the code.

(see structs.h) all the +1 ARRAY

Both solution fix the crash, and at the end the developper of TBA mud will make their choice.

Meanwhile, you fixed your crash.

The difference with >= will let you register 19 chars name instead of 20.
While MAX_ARRAY+1 seems more logical with the rest of the code.

Ah yes, because I still need room for the null terminator.

Thanks.

Castillo wrote: I did some tests:

Code:

#include "stdio.h" #include "string.h" #include "stdlib.h" int main() { const int MAX_NAME_LENGTH = 20; const char *chname = "duckduckduckduckduck"; char name [MAX_NAME_LENGTH]; strcpy(name, chname); return 0; }

Okay, this is interesting. I copied your code into a text editor in my terminal, compiled the first way, tested, deleted the a.out, recompiled the second way and had no difference!

I then implemented the following code, everything the same, but added strlen() and some logic to check for overflow.
Again, no differences during compile or running the code.


So either the OpenBSD compiler is doing something ruleswiae under the hood to strcpy() and strlen(), which I doubt, or there's an issue with the new compiler. I'll update my system to see if there's any changes if I recompile those codes. If not I might be asking the OpenBSD devs to kindly explain what's happening.

Addendum:

Just attempted the above steps in the refactored code but with strlen(chname) instead just in case strcpy() might have been cleaning up the buffer somehow. Again, same results.