Skip to content

[elfutils] create tmpfiles properly #7408

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

Merged
merged 5 commits into from
Sep 18, 2022

Conversation

evverx
Copy link
Contributor

@evverx evverx commented Mar 21, 2022

Now fuzz-libdwfl and fuzz-libelf can be run a few times in a row
with files triggering crashes.

It's another follow-up to #7395
and #7393.

@evverx evverx force-pushed the elfutils-tmpfiles branch from 1396628 to 40e33e1 Compare March 21, 2022 02:04
Copy link
Collaborator

@DavidKorczynski DavidKorczynski left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks for pointing out the tmp file creation optimisation.

@jonathanmetzman
Copy link
Contributor

Do we still want to land this?

Now fuzz-libdwfl and fuzz-libelf can be run a few times in a row
with files triggering crashes.

It's another follow-up to google#7395
and google#7393.
to make sure they are in more or less good shape
```
fuzz-libdwfl.c:54:10: error: unused variable 'res' [-Werror,-Wunused-variable]
  Dwarf *res = dwfl_module_getdwarf(mod, &bias);
         ^
1 error generated.
```

```
fuzz-dwfl-core.c:41:11: error: comparison of integers of different signs: 'ssize_t' (aka 'long') and 'size_t' (aka 'unsigned long') [-Werror,-Wsign-compare]
        assert(n == size);
               ~ ^  ~~~~
```
@evverx
Copy link
Contributor Author

evverx commented Sep 18, 2022

I thought the PR was merged (and even backported the fuzz targets downstream) but it seems I forgot to mark it "ready for review". I've just rebased it on top of the master branch and I think it should be good to go.

@evverx evverx marked this pull request as ready for review September 18, 2022 08:42
It should be kept elsewhere.
@evverx
Copy link
Contributor Author

evverx commented Sep 18, 2022

I also pushed a commit where I removed the fuzz-dwfl-core seed corpus.

@@ -38,7 +38,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
assert(fd >= 0);

n = write(fd, data, size);
assert(n == size);
assert(n == (ssize_t) size);

offset = lseek(fd, 0, SEEK_SET);
assert(offset == 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's as important to remove existing zips, the space is already taken up in the git history (I guess removing zips makes --depth 1 faster). But not adding new ones is good policy

Copy link
Contributor

@jonathanmetzman jonathanmetzman left a comment

Choose a reason for hiding this comment

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

lgtm

@jonathanmetzman jonathanmetzman merged commit 730ee7b into google:master Sep 18, 2022
@evverx
Copy link
Contributor Author

evverx commented Sep 18, 2022

I don't think it's as important to remove existing zips, the space is already taken up in the git history

Agreed. Without rewriting the history it doesn't help to save the space much. The idea was to make the elfutils project consistent with the new policy to for example prevent this pattern from being used if the build script is borrowed as a template for new integrations.

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.

3 participants