search

nosuchtim / keykit

KeyKit - an algorithmic MIDI scripting language and GUI system
Other
98 stars 7 forks source link

Need a getenv() to encapsulate safe use of mdep("env",...) #16

Open vizicist opened 1 year ago

vizicist commented 1 year ago

Any raw invocations of mdep("env",...) should be replace with a utility function getenv() that checks for the undefined value and returns "".

pbarada commented 1 year ago

Do you mean a bi_getenv (called from keykit code as "getenv") that returns "" if the environment variable isn't found? does getenv() exist in all ports of keykit (allowing bi_getenv to call getenv() directly instead of invokin 'mdep_mdep("env", "get", xxx)')?

pbarada commented 1 year ago

Actually there are direct calls to getenv() in main, so any port of keykit has to provide getenv()...

nosuchtim commented 1 year ago

Good point. I was originally referring to a getenv() in .k code. Probably still worthwhile as a general utility function in lib/util.k, so that .k code can use getenv() rather than mdep("env","get",...). Might be useful to have a test in tests/ to verify that getenv() is supported in a given port.

On Thu, Apr 6, 2023 at 12:24 PM pbarada @.***> wrote:

Actually there are direct calls to getenv() in main, so any port of keykit has to provide getenv()...

— Reply to this email directly, view it on GitHub https://github.com/nosuchtim/keykit/issues/16#issuecomment-1499514463, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABESNTAGER6NFHAMSZC7PTW74J7FANCNFSM6AAAAAAWJQDEIU . You are receiving this because you are subscribed to this thread.Message ID: @.***>

pbarada commented 1 year ago

Interesting. If I patch util2.k as follows: diff --git a/lib/util2.k b/lib/util2.k index 00a6c53..49c60cf 100644 --- a/lib/util2.k +++ b/lib/util2.k @@ -398,3 +398,12 @@ function forwardmidi(port1,port2) { print("MIDI IN! N = ",n) } } + +function __getenv(envvar) {

And then fire up keykit, it dies with "double free in tcache 2" with following backtrace:

...

6 0x00007ffff7aa0d7c in malloc_printerr (

str=str@entry=0x7ffff7bde710 "free(): double free detected in tcache 2")
at ./malloc/malloc.c:5664

7 0x00007ffff7aa312b in _int_free (av=0x7ffff7c19c80 ,

p=0x555555601fb0, have_lock=0) at ./malloc/malloc.c:4473

8 0x00007ffff7aa54d3 in __GI___libc_free (mem=mem@entry=0x555555601fc0)

at ./malloc/malloc.c:3391

9 0x00007ffff7a7eda7 in _IO_deallocate_file (fp=0x555555601fc0)

at ./libio/libioP.h:862

10 _IO_new_fclose (fp=fp@entry=0x555555601fc0) at ./libio/iofclose.c:74

11 0x000055555557bb21 in myfclose (f=f@entry=0x555555601fc0) at misc.c:47

12 0x0000555555575b17 in loadsymfile (s=s@entry=0x555555699030,

pushit=pushit@entry=1) at main.c:1501

13 0x00005555555870b5 in loadsym (s=s@entry=0x555555699030,

pushit=pushit@entry=1) at task.c:695

14 0x0000555555588424 in i_gvareval () at task.c:813

15 0x000055555558b75b in exectasks (nosetjmp=nosetjmp@entry=0) at task.c:660

16 0x0000555555576104 in main (argc=0, argv=0x5555555bba40 )

at main.c:1804

Note the "__getenv" in function name; did that to make sure execution is not invoking it. Since the free (frame #8) is getting called from fclose, I'm guessing something's smashing malloc's memory pool. Time to turn on memory debug to see if there's anything odd...

nosuchtim commented 1 year ago

I'm in San Diego through tomorrow, so may not be able to look at it for a bit.

On Thu, Apr 6, 2023 at 1:32 PM pbarada @.***> wrote:

Interesting. If I patch util2.k as follows: diff --git a/lib/util2.k b/lib/util2.k index 00a6c53..49c60cf 100644 --- a/lib/util2.k +++ b/lib/util2.k @@ -398,3 +398,12 @@ function forwardmidi(port1,port2) { print("MIDI IN! N = ",n) } } + +function __getenv(envvar) {

  • value = mdep("env", "get", envvar);

  • if (!defined(value))

  • {

  • value = ""

  • }

  • return value +}

And then fire up keykit, it dies with "double free in tcache 2" with following backtrace:

...

6 https://github.com/nosuchtim/keykit/issues/6 0x00007ffff7aa0d7c in

malloc_printerr ( @.***=0x7ffff7bde710 "free(): double free detected in tcache 2") at ./malloc/malloc.c:5664

7 https://github.com/nosuchtim/keykit/issues/7 0x00007ffff7aa312b in

_int_free (av=0x7ffff7c19c80 , p=0x555555601fb0, have_lock=0) at ./malloc/malloc.c:4473

8 https://github.com/nosuchtim/keykit/issues/8 0x00007ffff7aa54d3 in

__GI___libc_free @.***=0x555555601fc0) at ./malloc/malloc.c:3391

9 https://github.com/nosuchtim/keykit/issues/9 0x00007ffff7a7eda7 in

_IO_deallocate_file (fp=0x555555601fc0) at ./libio/libioP.h:862

10 https://github.com/nosuchtim/keykit/issues/10 _IO_new_fclose

@.***=0x555555601fc0) at ./libio/iofclose.c:74

11 https://github.com/nosuchtim/keykit/pull/11 0x000055555557bb21 in

myfclose @.***=0x555555601fc0) at misc.c:47

12 https://github.com/nosuchtim/keykit/pull/12 0x0000555555575b17 in

loadsymfile @.=0x555555699030, @.=1) at main.c:1501

13 https://github.com/nosuchtim/keykit/issues/13 0x00005555555870b5 in

loadsym @.=0x555555699030, @.=1) at task.c:695

14 https://github.com/nosuchtim/keykit/pull/14 0x0000555555588424 in

i_gvareval () at task.c:813

15 https://github.com/nosuchtim/keykit/pull/15 0x000055555558b75b in

exectasks @.***=0) at task.c:660

16 https://github.com/nosuchtim/keykit/issues/16 0x0000555555576104 in

main (argc=0, argv=0x5555555bba40 ) at main.c:1804

Note the "__getenv" in function name; did that to make sure execution is not invoking it. Since the free (frame #8 https://github.com/nosuchtim/keykit/issues/8) is getting called from fclose, I'm guessing something's smashing malloc's memory pool. Time to turn on memory debug to see if there's anything odd...

— Reply to this email directly, view it on GitHub https://github.com/nosuchtim/keykit/issues/16#issuecomment-1499589855, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABESNUQ4OUUX2VEPVIEE6TW74R5XANCNFSM6AAAAAAWJQDEIU . You are receiving this because you commented.Message ID: @.***>

pbarada commented 1 year ago

I was thinking of giving it a go, see what I can turn up...

pbarada commented 1 year ago

Turns out the sample code I gave you has "return value" which causes a syntax error "return without parenthesis" (why is that an error - grammer looks like it can handle return w/o parenthesis?) which causes flushfin() to get called from yyerror that closes the file - and on return from yyparse (regardless of error return) loadsymfile closes the file (again). Since yyerror() increments Error and calls flushfin, looking at modifying yyerror and loadsymfile to not close the file if Errors is non-zero. Other observations:

I'll put together a branch with changes for above...

nosuchtim commented 1 year ago

I'm not sure the MDEBUG stuff is worth maintaining (it was put in for some sort of debugging/optimization several decades ago (I love saying "decades" :-)), but if you can make it do something useful without too much trouble, go for it. updatelib(1) does take a long time, I'm not sure where the bottleneck is, but if plibrary improvements make it faster, that'd be great.

At some point in the near future I'll post a "State of Keykit" message to the mailing list, for a variety of reasons. One reason is to describe my various aborted attempts to port Keykit to Go (so it can be integrated more closely with my Palette software, among other reasons), and thoughts about the expressiveness of Keykit versus trying to do the same in Go. While I've created an OSC connection between the Palette and Keykit (which can certainly be useful), I'd really like to have a Keykit interpreter written in Go. My latest thought is that the goawk interpreter ( https://github.com/benhoyt/goawk, which recently moved to a bytecode approach) may be a very useful starting point for, essentially, a complete rewrite in Go that could still be code compatible with .k files. The other possibility is something like gop (https://github.com/goplus/gop) which provides operator overloading and is Go-compatible, but probably wouldn't be code-compatible. Basically, I'm happy to maintain the C version of Keykit and make sure it runs well and widely, and it's possible that that may end up being the only version of Keykit that ever exists, but I'll continue to try to move it to Go in some way/shape/form. Moving to WASM is also a goal. Adding some sort of namespace feature/hack to Keykit is probably the biggest language-related things I'll consider. One of the biggest impediments (or annoyances) to any porting is the use of setjmp/longjmp to recover from execution errors. It'll be interesting to see how/whether the goawk interpreter handles that in any way (or just punts).

 ...Tim...

On Fri, Apr 7, 2023 at 7:17 AM pbarada @.***> wrote:

Turns out the sample code I gave you has "return value" which causes a syntax error "return without parenthesis" (why is that an error - grammer looks like it can handle return w/o parenthesis?) which causes flushfin() to get called from yyerror that closes the file - and on return from yyparse (regardless of error return) loadsymfile closes the file (again). Since yyerror() increments Error and calls flushfin, looking at modifying yyerror and loadsymfile to not close the file if Errors is non-zero. Other observations:

  • Turning on MDEBUG causes problems(three debug messages per allocation which don't provide much information; needs cleanup)
  • for MDEBUG dbgallocate should make only one allocation that holds the buffer and prepends to it the mminfo struct for that allocation instead of separate malloc calls
  • All memory allocation tags should be static string/constants - to save a MDEBUG allocation just for the tag
  • plibrary needs improving (wrd1 and wrd2 should be local strings of BUFSIZ, not allocated since plibrary is called a huge number of times when executing 'updatelib(1)'
  • mix of calls to myfclose and fclose in source

I'll put together a branch with changes for above...

— Reply to this email directly, view it on GitHub https://github.com/nosuchtim/keykit/issues/16#issuecomment-1500330842, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABESNTKBG2GDVK6SHJPBDLXAAOWRANCNFSM6AAAAAAWJQDEIU . You are receiving this because you commented.Message ID: @.***>

pbarada commented 1 year ago

Comments inline

On 4/7/23 16:51, Tim Thompson wrote:

I'm not sure the MDEBUG stuff is worth maintaining (it was put in for some sort of debugging/optimization several decades ago (I love saying "decades" :-)), but if you can make it do something useful without too much trouble, go for it. updatelib(1) does take a long time, I'm not sure where the bottleneck is, but if plibrary improvements make it faster, that'd be great.

I have a pull request to get MDEBUG to produce sensible output again.  If you invoke key with "-d" (after building with MDEBUG defined in key.h) it will dump allocation/free with size/tag to the console and key.dbg.

The changes include improvement to plibrary to remove about 1000 allocations during "updatelib(1)", but major hitter during updatelib() is strsave called from substr(called 595K times). Perhaps adding "child" strings that are reference to "parent" strings which child contain address of parent string and offset/len fields, and if parent/child string needs to get modified then its replaced with a new string of the result. Is it worth adding "Implement child string references" to the issues list?

At some point in the near future I'll post a "State of Keykit" message to the mailing list, for a variety of reasons. One reason is to describe my various aborted attempts to port Keykit to Go (so it can be integrated more closely with my Palette software, among other reasons), and thoughts about the expressiveness of Keykit versus trying to do the same in Go. While I've created an OSC connection between the Palette and Keykit (which can certainly be useful), I'd really like to have a Keykit interpreter written in Go. My latest thought is that the goawk interpreter ( https://github.com/benhoyt/goawk, which recently moved to a bytecode approach) may be a very useful starting point for, essentially, a complete rewrite in Go that could still be code compatible with .k files. The other possibility is something like gop (https://github.com/goplus/gop) which provides operator overloading and is Go-compatible, but probably wouldn't be code-compatible. Basically, I'm happy to maintain the C version of Keykit and make sure it runs well and widely, and it's possible that that may end up being the only version of Keykit that ever exists, but I'll continue to try to move it to Go in some way/shape/form. Moving to WASM is also a goal. Adding some sort of namespace feature/hack to Keykit is probably the biggest language-related things I'll consider. One of the biggest impediments (or annoyances) to any porting is the use of setjmp/longjmp to recover from execution errors. It'll be interesting to see how/whether the goawk interpreter handles that in any way (or just punts).

...Tim...

On Fri, Apr 7, 2023 at 7:17 AM pbarada @.***> wrote:

Turns out the sample code I gave you has "return value" which causes a syntax error "return without parenthesis" (why is that an error - grammer looks like it can handle return w/o parenthesis?) which causes flushfin() to get called from yyerror that closes the file - and on return from yyparse (regardless of error return) loadsymfile closes the file (again). Since yyerror() increments Error and calls flushfin, looking at modifying yyerror and loadsymfile to not close the file if Errors is non-zero. Other observations:

  • Turning on MDEBUG causes problems(three debug messages per allocation which don't provide much information; needs cleanup)
  • for MDEBUG dbgallocate should make only one allocation that holds the buffer and prepends to it the mminfo struct for that allocation instead of separate malloc calls
  • All memory allocation tags should be static string/constants - to save a MDEBUG allocation just for the tag
  • plibrary needs improving (wrd1 and wrd2 should be local strings of BUFSIZ, not allocated since plibrary is called a huge number of times when executing 'updatelib(1)'
  • mix of calls to myfclose and fclose in source

I'll put together a branch with changes for above...

— Reply to this email directly, view it on GitHub https://github.com/nosuchtim/keykit/issues/16#issuecomment-1500330842, or unsubscribe

https://github.com/notifications/unsubscribe-auth/AABESNTKBG2GDVK6SHJPBDLXAAOWRANCNFSM6AAAAAAWJQDEIU . You are receiving this because you commented.Message ID: @.***>

— Reply to this email directly, view it on GitHub https://github.com/nosuchtim/keykit/issues/16#issuecomment-1500632744, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABMXGS477KTW3EM22YVEV6DXAB43NANCNFSM6AAAAAAWJQDEIU. You are receiving this because you commented.Message ID: @.***>

-- Peter Barada @.***

nosuchtim commented 1 year ago

major hitter during updatelib() is strsave called from substr(called 595K times) ... Is it worth adding "Implement child string references" to the issues list?

I wouldn't implement it with child string references, but an issue of "remove strsave from bi_substr" would be okay. A simple approach would be to have a static buffer (whose size is maintained with makeroom()) in bi_substr to eliminate the allocations (but not the copying of characters).

 ...Tim...