strings with twin characters

[ragnarok2040]

I ran across a bug that kind of worries me, but I haven't been able to reproduce it in a test case. I wrote some browsing code for a larger project, for browsing directories. It had been working, but, all of a sudden, I was unable to use strcat() to append a "/" to a folder with twin characters at the end, e.g. 123466, FOLDRR. I'm not sure if the problem lies in the strcpy() of fio_dirent_t member "name" copying an invalid character at the end or if it's actually strcat()'s fault. Replacing strcat() with sprintf() fixed the problem I was having, though, so it makes me think the strcpy() is fine. I appended the code of what exactly happens when browsing directories. This code works though, so I commented the parts that were giving me the trouble. I used the same optimization flags as well, thinking it might have been an optimization error.

Code: Select all

#include <stdio.h>
#include <string.h>
#include <libmc.h>
#include <sifrpc.h>
#include <sys/stat.h>
#include <loadfile.h>
#include <sbv_patches.h>

typedef struct &#123;
    char nothing&#91;32&#93;;
    int dircheck;
    char something&#91;256&#93;;
&#125; entries;

void LoadModules&#40;void&#41;
&#123;
    int ret;

    ret = SifLoadModule&#40;"rom0&#58;SIO2MAN", 0, NULL&#41;;
    if &#40;ret < 0&#41; &#123;
        printf&#40;"Failed to load module&#58; SIO2MAN"&#41;;
    &#125;

    ret = SifLoadModule&#40;"rom0&#58;MCMAN", 0, NULL&#41;;
    if &#40;ret < 0&#41; &#123;
        printf&#40;"Failed to load module&#58; MCMAN"&#41;;
    &#125;

    ret = SifLoadModule&#40;"rom0&#58;MCSERV", 0, NULL&#41;;
    if &#40;ret < 0&#41; &#123;
        printf&#40;"Failed to load module&#58; MCSERV"&#41;;
    &#125;
&#125;

static inline char* strzncpy&#40;char *d, char *s, int l&#41; &#123; d&#91;0&#93; = 0; return strncat&#40;d, s, l&#41;; &#125;

int main&#40;&#41; &#123;

    SifInitRpc&#40;0&#41;;

    sbv_patch_enable_lmb&#40;&#41;;
    sbv_patch_disable_prefix_check&#40;&#41;;

    LoadModules&#40;&#41;;

    mcInit&#40;MC_TYPE_MC&#41;;

    entries FileEntry&#91;2048&#93;;
    char path&#91;4096&#93;;

    strcpy&#40;path,"mc0&#58;/"&#41;;   

    int i,dd;
     
    int n = 0;
    fio_dirent_t buf;

    dd = fioDopen&#40;path&#41;; 

    while&#40;fioDread&#40;dd,&buf&#41;&#41; &#123; //list entries
        if&#40;&#40;FIO_SO_ISDIR&#40;buf.stat.mode&#41;&#41; && &#40;!strcmp&#40;buf.name,"."&#41; || !strcmp&#40;buf.name,".."&#41;&#41;&#41;
            continue;
        if&#40;FIO_SO_ISDIR&#40;buf.stat.mode&#41;&#41; &#123;
            FileEntry&#91;n&#93;.dircheck = 1;
            strcpy&#40;FileEntry&#91;n&#93;.something,buf.name&#41;; //filename
            strzncpy&#40;FileEntry&#91;n&#93;.nothing,FileEntry&#91;n&#93;.something,31&#41;; //displayname
            n++;
        &#125;
        if&#40;n>2046&#41; break;
    &#125;
    fioDclose&#40;dd&#41;;

    for&#40;i=0;i<&#40;n-1&#41;;i++&#41; &#123;
        strcpy&#40;path,"mc0&#58;/"&#41;;

        if&#40;strchr&#40;path,'/'&#41; == NULL&#41;  &#123;&#125;; //used for checking if coming from device list in browser

        strcat&#40;path,FileEntry&#91;i&#93;.something&#41;; //folder "i" selected
        strcat&#40;path,"/"&#41;; //append delimiter which wasn't working e.g. "mc0&#58;/FOLDRR" would still be in path afterwards

        if&#40;i>0&#41; //append something else to it which wasn't working
            strcat&#40;path,FileEntry&#91;i-1&#93;.something&#41;;
        else
            strcat&#40;path,FileEntry&#91;i+1&#93;.something&#41;;

        strcat&#40;path,"/"&#41;;

        printf&#40;"%s\n",path&#41;;
    &#125;

    return 0;
&#125;

Makefile

Code: Select all

# _____     ___ ____     ___ ____
#  ____|   |    ____|   |        | |____|
# |     ___|   |____ ___|    ____| |    \    PS2DEV Open Source Project.
#-----------------------------------------------------------------------
# Copyright 2001-2004, ps2dev - http://www.ps2dev.org
# Licenced under Academic Free License version 2.0
# Review ps2sdk README & LICENSE files for further details.
#
# $Id: Makefile.sample 1150 2005-06-12 11:42:08Z pixel $

EE_BIN = stringy.elf
EE_CFLAGS = -O3 -Winline -ffast-math -finline-functions -fstrict-aliasing -funsigned-char -fomit-frame-pointer -funroll-loops
EE_LIBS = -lmc -lpatches
EE_OBJS = stringy.o

all: $(EE_BIN)

clean:
	rm -f *.elf *.o

include $(PS2SDK)/samples/Makefile.pref
include $(PS2SDK)/samples/Makefile.eeglobal

[Jim]

Code: Select all

typedef struct { 
    char nothing[32]; 
    int dircheck; 
    char something[256]; 
} entries; 

int main() { 
...
    entries FileEntry[2048]; 
...

That's nearly 600Kb of stack - is that OK?

Jim

[ragnarok2040]

It seems to be alright, someone was able to browse more than a thousand or so files on his usb drive with it like that without any problems. I'd originally had it defined as entries *FileEntry = (entries*)malloc(sizeof(entries)*2048); but changed it recently to that as a local variable since malloc wasn't having any trouble allocating the memory, but calling free(FileEntry) kept randomly crashing the program.

Two instances of the function is all that's called at any one time... maybe I should just define it globally so it doesn't take up more than a megabyte of memory... Thanks for the heads up :D.

[Jim]

The malloc is a much better idea. Just because malloc can allocate it doesn't mean there's enough room on the stack for the same sized object - you probably need to tell the linker or the C runtime startup that you need such a huge stack - I don't know what the default is on PS2SDK, but I bet it's more like 64Kb than 1Mb.
If your stack overflowed, it could easily cause the problem you are having.

Jim

[ragnarok2040]

Looks like the stack size for ps2sdk is 128 Kb. I am overflowing the stack so I changed my code back, heh.

What I don't understand is why would it only affects double character ending strings only. It doesn't affect directories with 32 characters in the name or browsing multiple levels up or down. Compiling with -fstack-check didn't cause it to exit the program either. But I think that only works on Linux... and if not, I don't think I was browsing enough entries to overflow the stack, heh. Maybe that guy was using an older version :/.

Oh yeah, the bug still happens whether I'm malloc'ing FileEntry or relying on the stack.

I checked the disassembly and I think I came across something strange...
The first one is from the program using strcat with the bug and the second with sprintf().

Code: Select all

error with strcat
 940:	3c040000 	lui	a0,0x0
 944:	24840000 	addiu	a0,a0,0
 948&#58;	0c000000 	jal	0 <RomBrowserInput>

working sprintf
 940&#58;	3c060000 	lui	a2,0x0
 944&#58;	24c60000 	addiu	a2,a2,0
 948&#58;	00c0202d 	move	a0,a2
 94c&#58;	24e70024 	addiu	a3,a3,36
 950&#58;	268510c8 	addiu	a1,s4,4296
 954&#58;	0c000000 	jal	0 <RomBrowserInput>

I think this is the output from a working strcat operation that does something similar...

Code: Select all

  14:	3c030000 	lui	v1,0x0
  18:	8c820008 	lw	v0,8(a0)
  1c:	24650010 	addiu	a1,v1,16
  20:	03a0202d 	move	a0,sp
  24:	ffa70000 	sd	a3,0(sp)
  28&#58;	0c000000 	jal	0 <main>

I don't know assembly at all, but 0x0 would probably be the null terminator and I think the rest would be modification of the strings...

[Jim]

You can't tell anything from that.

You can easily check strcat. Right at the beginning of main() just do something like

Code: Select all

char test[2048] = "FOLDRR";
strcat(test, "banana");

and then check the result. If it's wrong, strcat is wrong. If it's right, then something else you are doing is wrong.

Jim

[ragnarok2040]

I did post in my first post that I couldn't reproduce it outside of the program. That last copy & paste of assembly was from a program just like the one you posted, except I used a string length of 256. You're right that the assembly code didn't say anything, heh. I'm not sure exactly what else I can be doing with strcpy and strcat that would make it go wrong only on specific strings with double character non-numeral endings, anyway.

The exact behavior of the bug with "1234MM" being the string inside the array of directory names is:

path[4096];

(output from my program is intermixed)
strcpy(path,"mc0:/");
path = mc0:/
strcat(path,"1234MM");
path = mc0:/1234MM
strcat(path,"/");
path = mc0:/1234MM
strcat(path,"filename.fil");
path = mc0:/1234MM
.. selected
there is a /
there is no /
strcpy(path,"path");
path = path

Edit:
It disappears if I use word, doubleword or quadword alignments (pretty much anything 16 bytes and over), so I'm guessing strcat tries to strcpy the second string to the first string at the wrong byte for some reason, maybe the byte after the null terminator... since I think if the position were earlier it would cut into the first string's characters or maybe the strcpy there is incrementing a spot prior to copying the string for some reason.

Edit2:
I am able to reproduce the exact behavior I'm getting by setting the first character of the src string to a null terminator prior to using strcat to append it to the dest string.

Edit3:
Having the dest string aligned to 16 bytes caused my program to start producing random crashes every once in a while when entering those specific directories...