Long names can cause crashes or redefinition warnings.

[robzed]

I'm adding to pForth to make it able to use SDL. I'm creating a C function bindings (rf_custom.c style) and then add add forth words to complete this so that I can use SDL2 from pForth. (This is based on ProgrammingRainbow gForth's work).

The custom c function bindings all work. I'm not adding constants and structures in Forth. The problem appears with constants like this one:

#3 constant SDL_THREAD_PRIORITY_TIME_CRITICAL

Some of the names in SDL are very long:
"SDL_THREAD_PRIORITY_TIME_CRITICAL" 33 bytes
"SDL_THREAD_PTHREAD_RECURSIVE_MUTEX" 34 bytes
"SDL_VIDEO_DRIVER_WAYLAND_QT_TOUCH" 33 bytes
"SDL_AUDIO_ALLOW_FREQUENCY_CHANGE" 32 bytes

NOTE: These are not the only ones... just the ones that were causing redefinition warnings.

For all of them (all constant), I got redefinition warnings. Some other long names I haven't.

For the last one I get a crash:
include added 896 bytes,28292 left.
Include SDL2/SDL_audio.fs
SDL_AUDIO_ALLOW_FREQUENCY_CHANGE redefined.
zsh: segmentation fault /Users/rob/Current_Projects/pForth/pforth/platforms/unix/pforth_standalone

---

I also notice that
1 constant SDL_AUDIO_ALLOW_FREQUENCY

Appears correctly in the dictionary as
words SDL_AUDIO_ALLOW_FREQUENCY
<>>

I also notice that
1 constant SDL_AUDIO_ALLOW_FREQUENCY_CHANGE123456789 ok

Appears in the dictionary as
words SDL_AUDIO SDL_AUDIO_ALLOW_FREQUENCY
<>

So in summary:

• I'm going to need to extend the word length if there is a constraint.
• I'm definite going to need to find the crash!

[robzed]

I assume this is the name length field, with flags as the top bits?
#define FLAG_PRECEDENCE (0x80)
#define FLAG_IMMEDIATE (0x40)
#define FLAG_SMUDGE (0x20)
#define MASK_NAME_SIZE (0x1F)

What's FLAG_PRECEDENCE used for?

[robzed]

I'm not sure why it's crashing, but it seems like that the names being bigger than 31 characters is causing the problem, and there is no warning in place to detect this problem.

For the moment I think I'm going to comment out FLAG_PRECEDENCE, and move FLAG_SMUDGE into that location.

/* #define FLAG_PRECEDENCE (0x80) */
#define FLAG_IMMEDIATE (0x40)
#define FLAG_SMUDGE (0x80)
#define MASK_NAME_SIZE (0x3F)

I'll also need to alter the forth version in misc1.fth:
$80 constant FLAG_SMUDGE
(by the way, why was there a space $ 20 constant FLAG_SMUDGE ??)

31 constant LV_MAX_CHARS \ maximum number of letters in name
becomes
63 constant LV_MAX_CHARS \ maximum number of letters in name

: ID. ( nfa -- )
count 31 and type
;
becomes
: ID. ( nfa -- )
count 63 and type
;

inside : F?.SEARCH.NFA
nfa count 31 and
becomes
nfa count 63 and

: N>NEXTLINK ( nfa -- nextlink , traverses name field )
dup c@ 31 and 1+ + aligned
;
becomes
: N>NEXTLINK ( nfa -- nextlink , traverses name field )
dup c@ 63 and 1+ + aligned
;

inside ffFindNFA()

This
WordLen = (uint8_t) ((ucell_t)*WordName & 0x1F);
becomes
WordLen = (uint8_t) ((ucell_t)*WordName & MASK_NAME_SIZE);

: PARTIAL.MATCH.NAME ( $str1 nfa -- flag , is $str1 in nfa ??? )
count $ 1F and
becomes
: PARTIAL.MATCH.NAME ( $str1 nfa -- flag , is $str1 in nfa ??? )
count $ 3F and

inside
void TypeName( const char *Name )
Len = *Name & 0x1F;
becomes
Len = *Name & 0x3F;

[robzed]

Probably best to extend these as well...

void CreateDicEntryC( ExecToken XT, const char *CName, ucell_t Flags )
{
ForthString FName[70]; // was 40

void CreateDicEntryC( ExecToken XT, const char *CName, ucell_t Flags )
{
ForthString FName[70]; // was 40

static void CreateDeferredC( ExecToken DefaultXT, const char *CName )
{
char FName[70]; // was 40

[robzed]

So all the redefinition warnings are gone.
But I still get a crash, maybe at a different line, but same file. I eliminated most of my code - by changing first word on the line does a postpone \ ... so line shoudl be commented out.

zsh: segmentation fault pforth/platforms/unix/pforth_standalone

run it under lldb

Process 49215 stopped

• thread Fix readme #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x40f0a218)
  frame #0: 0x0000000100007f70 pforth_standalone`Word [inlined] ffSkip(AddrIn=, Cnt=138, c=' ', AddrOut=) at pf_words.c:87:16 [opt]
  84 if( c == BLANK )
  85 {
  86 while( ( Cnt > 0 ) &&
  -> 87 (( *s == BLANK) || ( *s == '\t')) )
  88 {
  89 DBUGX(("ffSkip BLANK: %c, %d\n", *s, Cnt ));
  90 s++;
  Target 0: (pforth_standalone) stopped.
  warning: pforth_standalone was compiled with optimization - stepping may behave oddly; variables may not be available.

[robzed]

(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x40f0a218)
  * frame #0: 0x0000000100007f70 pforth_standalone`Word [inlined] ffSkip(AddrIn=<unavailable>, Cnt=138, c=' ', AddrOut=<unavailable>) at pf_words.c:87:16 [opt]
    frame #1: 0x0000000100007f58 pforth_standalone`Word(c=' ', Upcase=0) at pf_words.c:221:10 [opt]
    frame #2: 0x00000001000080d4 pforth_standalone`ffLWord(c=<unavailable>) at pf_words.c:252:10 [opt]
    frame #3: 0x000000010000a3dc pforth_standalone`ffInterpret at pfcompil.c:845:19 [opt]
    frame #4: 0x000000010000a8d8 pforth_standalone`ffOuterInterpreterLoop at pfcompil.c:930:21 [opt]
    frame #5: 0x000000010000ab5c pforth_standalone`ffIncludeFile(InputFile=0x00000001ef308bf8) at pfcompil.c:953:17 [opt]
    frame #6: 0x000000010000518c pforth_standalone`pfCatch(XT=<unavailable>) at pf_inner.c:1251:23 [opt]
    frame #7: 0x000000010000a3b8 pforth_standalone`ffInterpret [inlined] FindAndCompile(theWord="3SDL_CONVERTAUDIO SDL_CONVERTAUDIO A -- N\t( CVT -- )- N\t( CVT SRC_FORMAT SRC_CHANNELS SRC_RATE DST_FORMAT DST_

(lldb) p gCurrentTask->td_SourceNum
(cell_t) 138
(lldb) p gCurrentTask->td_SourcePtr
(char *) 0x0000000040f0a218 ""
(lldb) p gCurrentTask->td_LineNumber
(cell_t) 92
(lldb) p gCurrentTask->td_OUT
(cell_t) 0
(lldb) p &gCurrentTask->td_TIB
(char (*)[256]) 0x0000000140f0a218
(lldb) p gCurrentTask->td_TIB
(char[256]) "c-function SDL_NewAudioStream SDL_NewAudioStream n n n n n n -- a\t( src_format src_channels src_rate dst_format dst_channels dst_rate -- )-- )"

NOTE: c-function is currently defined as:
: c-function ( "forth-name" "c-name" "{type}" "--" "type" -- )
postpone
;

line 92 looks like this:
c-function SDL_NewAudioStream SDL_NewAudioStream n n n n n n -- a ( src_format src_channels src_rate dst_format dst_channels dst_rate -- )

file pforth_standalone
pforth_standalone: Mach-O 64-bit executable arm64

[robzed]

How is the TIB at 00x0000000_1_40f0a218 but the td_SourcePtr is at (char *) 0x0000000040f0a218 ?

The lower 32 bits being the same can't be a coincidence...

[robzed]

Lines immediately before this crash (this was generated by setting 1 echo ! before doing the include.

	c-pointer:  SDL_AudioSpec-userdata
end-structure
\ struct SDL_AudioCVT
begin-structure SDL_AudioCVT
	c-int:          SDL_AudioCVT-needed
    c-uint16:       SDL_AudioCVT-src_format
	c-uint16:       SDL_AudioCVT-dst_format
	c-double:       SDL_AudioCVT-rate_incr
	c-uint8-ptr:    SDL_AudioCVT-buf
	c-int:          SDL_AudioCVT-len
	c-int:          SDL_AudioCVT-len_cvt
	c-int:          SDL_AudioCVT-len_mult
	c-double:       SDL_AudioCVT-len_ratio
	10 c-func-ptrs: SDL_AudioCVT-filters
	c-int:          SDL_AudioCVT-filter_index
end-structure
\ ------===< functions >===-------
c-function SDL_GetNumAudioDrivers SDL_GetNumAudioDrivers  -- n	( -- )
c-function SDL_GetAudioDriver SDL_GetAudioDriver n -- a	( index -- )
c-function SDL_AudioInit SDL_AudioInit a -- n	( driver_name -- )
c-function SDL_AudioQuit SDL_AudioQuit  -- void	( -- )
c-function SDL_GetCurrentAudioDriver SDL_GetCurrentAudioDriver  -- a	( -- )
c-function SDL_OpenAudio SDL_OpenAudio a a -- n	( desired obtained -- )
c-function SDL_GetNumAudioDevices SDL_GetNumAudioDevices n -- n	( iscapture -- )
c-function SDL_GetAudioDeviceName SDL_GetAudioDeviceName n n -- a	( index iscapture -- )
c-function SDL_GetAudioDeviceSpec SDL_GetAudioDeviceSpec n n a -- n	( index iscapture spec -- )
c-function SDL_GetDefaultAudioInfo SDL_GetDefaultAudioInfo a a n -- n	( name spec iscapture -- )
c-function SDL_OpenAudioDevice SDL_OpenAudioDevice a n a a n -- n	( device iscapture desired obtained allowed_changes -- )
c-function SDL_GetAudioStatus SDL_GetAudioStatus  -- n	( -- )
c-function SDL_GetAudioDeviceStatus SDL_GetAudioDeviceStatus n -- n	( dev -- )
c-function SDL_PauseAudio SDL_PauseAudio n -- void	( pause_on -- )
c-function SDL_PauseAudioDevice SDL_PauseAudioDevice n n -- void	( dev pause_on -- )
c-function SDL_LoadWAV_RW SDL_LoadWAV_RW a n a a a -- a	( src freesrc spec audio_buf audio_len -- )
c-function SDL_FreeWAV SDL_FreeWAV a -- void	( audio_buf -- )
c-function SDL_BuildAudioCVT SDL_BuildAudioCVT a n n n n n n -- n	( cvt src_format src_channels src_rate dst_format dst_channels dst_rate -- )
c-function SDL_ConvertAudio SDL_ConvertAudio a -- n	( cvt -- )
zsh: segmentation fault  /Users/rob/Current_Projects/pForth/pforth/platforms/unix/pforth_standalone

[robzed]

Build and linked with -g -fsanitize=address

==57945==ERROR: AddressSanitizer: heap-use-after-free on address 0x6100000005d8 at pc 0x000100010104 bp 0x00016fdfe130 sp 0x00016fdfe128
READ of size 1 at 0x6100000005d8 thread T0
#0 0x100010100 in Word pf_words.c:221
#1 0x100010188 in ffLWord pf_words.c:252
#2 0x100014310 in ffInterpret pfcompil.c:845
#3 0x100015298 in ffOuterInterpreterLoop pfcompil.c:930
#4 0x100015964 in ffIncludeFile pfcompil.c:953
#5 0x100006e0c in pfCatch pf_inner.c:1251
#6 0x1000148ac in ffInterpret pfcompil.c:860
#7 0x100015298 in ffOuterInterpreterLoop pfcompil.c:930
#8 0x100015964 in ffIncludeFile pfcompil.c:953
#9 0x100006e0c in pfCatch pf_inner.c:1251
#10 0x1000148ac in ffInterpret pfcompil.c:860
#11 0x100015298 in ffOuterInterpreterLoop pfcompil.c:930
#12 0x100015964 in ffIncludeFile pfcompil.c:953
#13 0x100006e0c in pfCatch pf_inner.c:1251
#14 0x1000148ac in ffInterpret pfcompil.c:860
#15 0x100015298 in ffOuterInterpreterLoop pfcompil.c:930
#16 0x100005114 in pfQuit pf_core.c:329
#17 0x100005a5c in pfDoForth pf_core.c:550
#18 0x10000bdcc in main pf_main.c:145
#19 0x1856a0270 ()

(allocated and freed tables not helpful)

(lldb) p gCurrentTask->td_SourcePtr
(char ) 0x00006100000005d8 "/MPSBenchmarkLoop"
(lldb) p gCurrentTask->td_IN
(cell_t) 0
(lldb) p gCurrentTask->td_SourceNum
(cell_t) 138
(lldb) p gCurrentTask->td_IN
(cell_t) 0
(lldb) p gCurrentTask->td_TIB
(char[256]) "c-function SDL_NewAudioStream SDL_NewAudioStream n n n n n n -- a\t( src_format src_channels src_rate dst_format dst_channels dst_rate -- )-- )"
(lldb) p &gCurrentTask->td_TIB
(char ()[256]) 0x00006130000005d8

[robzed]

In the function:

static cell_t readLineFromStream( char *buffer, cell_t maxChars, FileStream *stream )
{
    int   c;
    int   len;
    char *p;
    static int lastChar = 0;
    int   done = 0;

DBUGX(("readLineFromStream(0x%x, 0x%x, 0x%x)\n", buffer, len, stream ));
    p = buffer;
    len = 0;
    while( (len < maxChars) && !done )
    {
        c = sdInputChar(stream);
        switch(c)
        {
            case EOF:
                DBUG(("EOF\n"));
                done = 1;
                if( len <= 0 ) len = -1;
                break;

            case '\n':
                DBUGX(("EOL=\\n\n"));
                if( lastChar != '\r' ) done = 1;
                break;

            case '\r':
                DBUGX(("EOL=\\r\n"));
                done = 1;
                break;

            default:
                *p++ = (char) c;
                len++;
                break;
        }
        lastChar = c;
    }

/* NUL terminate line to simplify printing when debugging. */
    if( (len >= 0) && (len < maxChars) ) p[len] = '\0';

    return len;
}

The line at the end: if( (len >= 0) && (len < maxChars) ) p[len] = '\0';

Has a zero setting in the wrong place - potentially past the end of the buffer: p[len] = 0

This should either be buffer[len] = 0; or *p = 0; I think....

[robzed]

Pull request raised for segmentation fault.

[robzed]

I have a change a branch of my fork that gives 63 length names. If you are interested in this, or in my making it switchable - so that it's 31 by default or 63 by compilation, then let me know.
I'd also like to put name-to-long warning or abort into the code, but I haven't got around to that yet.
https://github.com/robzed/pforth_SDL/tree/SDL2_working_branch

[philburk]

@robzed - Great detective work! Thanks so much for finding that p[len] bug.
And thanks for the fix as a separate small PR. That ws a nice xmas gift.

Also I am interested in the 63 character names as an option. I need to think about that FLAG_PRECEDENCE.

[robzed]

Give me a few days, and I'll look at making a PR for the optional name size - along with some warning about long names.

[philburk]

The crash from p[lan] is fixed.
Keeping this open to track the "Long name" support.

[philburk]

I looked into the FLAG_PRECEDENCE bit.
It is not referenced in pForth!

I found mention of a "precedence bit" on the Forth Inc site.
https://www.forth.com/starting-forth/11-forth-compiler-defining-words/

They seem to use the "precedence bit" the way we use FLAG_IMMEDIATE.
So I think it was defined for no good reason. I think it is safe to remove FLAG_PRECEDENCE,
shift the FLAG_IMMEDIATE and FLAG_SMUDGE left,
and increase the name size mask from $1F to $3F.

But note that this will break the compatibility between pForth executable and saved pForth dictionary files!
So if we do this then we also have to increment the PF_FILE_VERSION and PF_EARLIEST_FILE_VERSION.
I also see that I failed to update them when I added some IDs recently!

[robzed]

Added #182

Name to long warnings/errors to be raised as a separate PR.