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

i#2051: in frontend, check for execute not read of DR root dir #2389

Merged
merged 2 commits into from
Mar 10, 2021

Conversation

jwysowski
Copy link
Contributor

Adds a new static function file_is_executable in frontend.c

Fixes #2051

Adds a new static function file_is_executable in frontend.c

Fixes DynamoRIO#2051
@johnfxgalea
Copy link
Contributor

johnfxgalea commented Mar 8, 2021

Thank you @jwysowski for the PR!

LGTM!

@johnfxgalea
Copy link
Contributor

Perhaps, @derekbruening could also take a look since he raised the issue and maybe has an alternative fix in mind?

Copy link
Contributor

@derekbruening derekbruening left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you for the patch!

@derekbruening
Copy link
Contributor

Hmm, so why aren't the GA CI jobs running? B/c this is a PR from a forked repo? I thought the yaml had the right triggers for that and we even tested it on the DR repo.

@derekbruening
Copy link
Contributor

Hmm, so why aren't the GA CI jobs running? B/c this is a PR from a forked repo? I thought the yaml had the right triggers for that and we even tested it on the DR repo.

@abhinav92003 any ideas? This repo has the same triggers as the DR repo workflows:

on:
  # Run on pushes to master and on pull request changes, including from a
  # forked repo with no "push" trigger, while avoiding duplicate triggers.
  push:
    branches:
      - master
  pull_request:
    types: [opened, reopened, synchronize]

@abhinav92003
Copy link

@abhinav92003 any ideas? This repo has the same triggers as the DR repo workflows:

DynamoRIO CI trigger for fork repo PRs can be seen in dynamorio/PR#4634.

I tried doing it again in dynamorio/PR#4771, and it still works. Though let me try syncing my forked repo to latest DR and re-try.

@derekbruening
Copy link
Contributor

@abhinav92003 any ideas? This repo has the same triggers as the DR repo workflows:

DynamoRIO CI trigger for fork repo PRs can be seen in dynamorio/PR#4634.

I tried doing it again in dynamorio/PR#4771, and it still works. Though let me try syncing my forked repo to latest DR and re-try.

Huh, I wonder if there's sthg different in drmemory repo: though the .yml looks identical. There aren't any other settings beyond the .yml right (and what branches are marked as required)? I can do a test try for drmemory repo later today.

@derekbruening
Copy link
Contributor

My fork test for drmemory repo is running the tests: #2390.
Could there be something where you and I have privileges in the original repo?
Have there been any dynamorio repo PR's from non-member forks since we put in GA CI -- maybe not, since khuey is a project member now.

@abhinav92003
Copy link

This seems relevant: https://github.community/t/run-a-github-action-on-pull-request-for-pr-opened-from-a-forked-repo/16054

Particularly,

The actions are in fact executed, but it happens against the _fork_, not against the base repo.

This does require that the forked repo also has actions enabled though.

@abhinav92003
Copy link

@jwysowski On the Settings page for your forked repository, see Actions on the left-hand side and please confirm whether actions are allowed.

@jwysowski
Copy link
Contributor Author

Actions weren't enabled in my forked repository.

@abhinav92003
Copy link

Actions weren't enabled in my forked repository.

After enabling them, please re-trigger CI on this branch. The way to do that would probably be either using the Actions tab on your forked repo, but I'm not sure if that'll reflect on this PR. If it doesn't work, then try submitting another commit to this branch.

@jwysowski
Copy link
Contributor Author

Ok, now everything is fine

@johnfxgalea johnfxgalea merged commit a8c9721 into DynamoRIO:master Mar 10, 2021
@johnfxgalea
Copy link
Contributor

@jwysowski Thanks for the path.

@abhinav92003 Thank you for investigating the issue regarding the CI.

@jwysowski jwysowski deleted the i2051-check-for-execute branch March 10, 2021 14:11
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.

in frontend, check for execute not read of DR root dir
4 participants