Page MenuHomeFreeBSD

Change image activator to check length of interpreter is less then MAXPATHLEN.
Needs ReviewPublic

Authored by ambrisko on Jun 6 2018, 10:56 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

The image activator checks to see if the space for the path of the interpreter
is larger then MAXPATHLEN. This is bad since it should check the length
of the interpreter is less then MAXPATHLEN. Update the error message to
give a better idea of why a binary won't run.

Also on failure, only free interp_buf if it is not NULL to prevent double
free. Do the VOP_UNLOCK even if it is not locked. I'm not sure about
this but the success part does this so I'm following suite.

Test Plan

Ran cross compile tools that had a long path name to the interpreter that
now works. Didn't do negative testing.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 18557
Build 18256: arc lint + arc unit

Event Timeline

In what situation is this change needed?

sys/kern/imgact_elf.c
851

Now userland can specify an arbitrarily large size to a malloc call.

867

The interp != NULL check doesn't seem to be needed.

868

"too"

1092–1093

How can this be preventing a double free?

  1. Updating D15683: Change image activator to check length of interpreter is less then MAXPATHLEN. #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment. #
  4. If you intended to create a new revision, use:
  5. $ arc diff --create

Address markj's feedback.

Limit the malloc to the MAXPATHLEN. Add in a failure check after malloc
and not later on. Change "to" to "too".

Clarify that when errors happen, the malloc may not have happened so then
there is no memory to free (ie. goto ret; calls when malloc didn't happen).

Thanks for the feedback.

sys/kern/imgact_elf.c
847

Extra semicolon.

852

malloc(M_WAITOK) can't fail.

1092–1093

This part of the change is not needed.

Get rid of the malloc check and spurious ';'.

Thanks for the feedback.

ambrisko added inline comments.
sys/kern/imgact_elf.c
1092–1093

Are you saying there is no way the goto ret's or interp = imgp->image_header)
+ phdr[i].p_offset is done bypassing the malloc?

sys/kern/imgact_elf.c
1092–1093

No, but free(NULL) is a no-op.

Remove NULL check since free does that.