Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dash parser fixes #18

Merged
merged 2 commits into from
May 12, 2020
Merged

Dash parser fixes #18

merged 2 commits into from
May 12, 2020

Conversation

tucak
Copy link
Contributor

@tucak tucak commented Dec 19, 2019

This is for handling cases like ${}, ${,}, ${+}, etc. Without this, they would end up in an endless mutual recursion of shim.parse_arg and shim.arg_char, ending with a stack overflow:

Fatal error: exception Stack overflow
Raised by primitive operation at file "shim.ml", line 277, characters 22-34
Called from file "shim.ml", line 253, characters 20-54
Called from file "shim.ml", line 305, characters 25-67
Called from file "shim.ml", line 305, characters 25-67

...repeating endlessly.

With this patch, the output is:

smoosh: bad substitution

Instead of blowing up the stack, let the user know that there was an
error.
Smoosh mishandled Dash's memory routines that could lead to buffer
overflows when executing nested evals.

After an eval is done, the stackmark must be popped before the file is
popped and the input string is unallocated. Skipping the stack popping
can lead to situations where Dash mistakenly believes that the free
space spans multiple stack blocks.

This change makes Smoosh pop the stackmark before unallocating.
@tucak tucak changed the title Handle malformed substitutions gracefully Dash parser fixes Feb 8, 2020
@tucak
Copy link
Contributor Author

tucak commented Feb 8, 2020

I am putting this here, otherwise there would be merge conflicts.

Another crash caused by miscommunication between Smoosh and Dash. This time a buffer overflow.

Reproducer:

force="echo 'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA\
AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA\
AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA\
AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA\
AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA\
AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA\
AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA\
AAAAAAAAAAAAA' >/dev/null"

eval "eval ${force}"

eval "${force}"

Why does it fail?

In Dash, the order of memory operations when executing eval is:

  • sstrdup
  • setinputstring
  • setstackmark
  • do whatever needs to be done...
  • popstackmark
  • popfile
  • stunalloc

In Smoosh, one of the pieces was missing:

  • sstrdup
  • setinputstring
  • setstackmark
  • do whatever needs to be done...
  • popfile
  • stunalloc

The problem with this is that in Dash, stack blocks can only be deallocated using popstackmark. stunalloc assumes that the string being "unallocated" is in the currently active stack block, and just sets the next free byte to be the beginning of the string. This can lead to buffer overflows.

After the inner eval, the stack blocks look like the following:

                               stacknxt       sstrend
                                  v              v
    prev  .--------------.  prev  .--------------.
  <-------|  outer eval  |<-------|  inner eval  |
          '--------------'        '--------------'

Where stacknxt points to the next free byte, and sstrend to the end of the allocation. The active stack block is the one belonging to inner eval.

When the outer eval is done, the situation turns into the following:

       stacknxt                               sstrend
          v                                      v
    prev  .--------------.  prev  .--------------.
  <-------|  outer eval  |<-------|  inner eval  |
          '--------------'        '--------------'

The block of inner eval is still the active block, but the next free byte is now pointing to the beginning of outer eval. Everything between the two is viewed as free memory by Dash, and the next eval string, if longer than the original outer eval string, will happily write beyond the area allocated for outer eval.

Popping the stackmark like Dash does fixes this issue.

mgree added a commit to binpash/libdash that referenced this pull request May 12, 2020
@mgree mgree merged commit 04e5810 into mgree:master May 12, 2020
@mgree
Copy link
Owner

mgree commented May 12, 2020

Whew---I'm finally getting to these! I'm sorry to have taken so long.

Travis is being weird (stuck in an old .travis.yml file), but the tests look good here. Thank you!!!!!

@tucak tucak deleted the subst-fix branch August 29, 2020 18:51
@mgree mgree mentioned this pull request Dec 23, 2020
mgree added a commit to binpash/libdash that referenced this pull request Feb 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants