search

wryun / es-shell

es: a shell with higher-order functions
http://wryun.github.io/es-shell/
Other
307 stars 25 forks source link

Fix bugs revealed by GCDEBUG. #52

Closed jpco closed 10 months ago

jpco commented 1 year ago

Three classes of error with GCDEBUG (or just GCALWAYS), which probably represent latent memory unsafety:

segfault in $&fsplit (which causes crash on startup)
segfault in $&catch (which causes crash on exit)
segfault in tilde handling (which causes either a crash or the inability to actually expand ~ to the output of %home)

It's unbelievable that I've taken 5-6 years to actually submit this pull request. Also sorry for being bad at Github and sending this a couple times.

Fixes https://github.com/wryun/es-shell/issues/18

wryun commented 1 year ago

In fairness, the bug report has been sitting there for that long and I haven't done anything about it :)

Thanks! Hopefully it won't take be 5-6 years to review this...

mwgamera commented 1 year ago

Do we care about mixing declarations and code? I'm not sure which version of C we target, but I was under impression the rest of the code base doesn't mix them. The splitstring_r function is not reentrant in the conventional sense of the word (and doesn't need to be), so the comment that says it is might be misleading. Other than that it looks okay to me and seems to be fixing what it claims to be fixing in a reasonable way.

jpco commented 1 year ago

Admittedly, I've always been really sloppy with C versions. It would be a good idea to start compiling with something like -ansi if folks agree that's the flag to set (Well, actually -- I just tried it and the compiler immediately barfed on S_IFREG and friends in access.c. Hmm. I'll just use -Wdeclaration-after-statement for now.)

I agree splitstring_r was incorrect. I renamed it to stepsplit, to correspond with startsplit and endsplit.

memreflect commented 1 year ago

Regarding C versions, it has probably been C with compiler extensions for a while.

I downloaded the version 0.84 tarball for example, and the code for $&access declares a new list near the end of the function; the mailing list archives indicate that version was announced in April 1993, only 3-4 years after C89/C90 was ratified and 6 years before C99.

mwgamera commented 1 year ago

@memreflect: Ref macro starts a new block.

memreflect commented 1 year ago

@mwgamera: Of course it does... Thank you for pointing out my mistake.

jpco commented 10 months ago

@wryun , would you be willing to take a look at this? I addressed everything in https://github.com/wryun/es-shell/pull/52#issuecomment-1509860293.

wryun commented 10 months ago

Ok, if you're confident! I'm generally terrified of C string handling bugs, and even with you cleaning it up (nice) it still has a few too many conditionals for my liking, but without a proper test suite...