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

Fix testcase set_filename #1092

Merged
merged 18 commits into from
Mar 22, 2023
Merged

Conversation

SpaceWhite
Copy link
Contributor

@SpaceWhite SpaceWhite commented Feb 23, 2023

fix #1084

add rename file logic in set_filename when new file name is set

self.filename = Some(filename);

match self.store_input() {
Copy link
Member

Choose a reason for hiding this comment

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

If we store, do we want to remove the old version (if that exists)?

Copy link
Contributor Author

@SpaceWhite SpaceWhite Feb 23, 2023

Choose a reason for hiding this comment

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

To remove old file versions, we could implement a delete function for the Input trait.
But I'm wondering why this issue is even a problem on the first place?
Isn't set_filename is only called when testcase.filename does not exist? Hence it is impossible to have self.filename with some string value.

@VTCAKAVSMoACE

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the solution here is to check if the file .<filename>.lafl_lock exists. If it does not, then it is safe to remove. Otherwise, return an illegal state error.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe in this case, we also must not simply store_input but actually to move the input file, the .metadata file, and the .lafl_lock (if each exist). Otherwise, this will simply create a copy.

Copy link
Member

Choose a reason for hiding this comment

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

wouldn't we maybe have created the lock file, though?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, we would have made the lockfile. But even so, if we are renaming to something that we currently use, it would be unsafe to remove/replace the destination file.

Copy link
Member

Choose a reason for hiding this comment

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

So what to do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added rename file logic. If either old lock file or new lock file exist, then set_filename will return error.

Copy link
Member

Choose a reason for hiding this comment

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

Will that ever work, then? When is the lock file removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added some more code to the functions.
So the idea is that old/new lock files are created and deleted in set_filename function.

If lock file are already created for old/new testcase then set_filename called, then it throws an error.
By doing this testcases could be safely created and removed.

@@ -4,6 +4,9 @@
use alloc::string::String;
use core::{default::Default, option::Option, time::Duration};

#[cfg(feature = "std")]
use std::{fs, path::Path};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it okay for testcase.rs to depend on std?

Copy link
Member

Choose a reason for hiding this comment

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

It needs to work without (for inmemory), but for storing to disk it needs std yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added set_Filename function for no_std.

Since there is no file creation/deletion, it just assigns value to struct.

@domenukk
Copy link
Member

clippy complains

error: the borrowed expression implements the required traits
   --> libafl/src/corpus/testcase.rs:176:24
    |
176 |             fs::rename(&old_filename, &new_filename)?;
    |                        ^^^^^^^^^^^^^ help: change this to: `old_filename`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
note: the lint level is defined here
   --> libafl/src/lib.rs:16:9
    |
16  | #![deny(clippy::all)]
    |         ^^^^^^^^^^^
    = note: `#[deny(clippy::needless_borrow)]` implied by `#[deny(clippy::all)]`

error: the borrowed expression implements the required traits
   --> libafl/src/corpus/testcase.rs:176:39
    |
176 |             fs::rename(&old_filename, &new_filename)?;
    |                                       ^^^^^^^^^^^^^ help: change this to: `new_filename`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow

error: the borrowed expression implements the required traits
   --> libafl/src/corpus/testcase.rs:180:24
    |
180 |             fs::rename(&old_metadata_filename, &new_metadata_filename)?;
    |                        ^^^^^^^^^^^^^^^^^^^^^^ help: change this to: `old_metadata_filename`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow

error: the borrowed expression implements the required traits
   --> libafl/src/corpus/testcase.rs:180:48
    |
180 |             fs::rename(&old_metadata_filename, &new_metadata_filename)?;
    |                                                ^^^^^^^^^^^^^^^^^^^^^^ help: change this to: `new_metadata_filename`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow

@SpaceWhite
Copy link
Contributor Author

@domenukk

fixed

@domenukk
Copy link
Member

domenukk commented Mar 9, 2023

@addisoncrump do you think this is correct now? thx :)

@domenukk
Copy link
Member

Sorry, missed the update. Looks good to me now, thanks! 👍

@domenukk domenukk merged commit a659dd8 into AFLplusplus:main Mar 22, 2023
@SpaceWhite SpaceWhite deleted the testcase-set_filename branch March 23, 2023 00:58
fengjixuchui added a commit to fengjixuchui/LibAFL that referenced this pull request Mar 23, 2023
@tokatoka
Copy link
Member

fuzzbench_qemu broken after this

tokatoka added a commit that referenced this pull request Mar 23, 2023
tokatoka added a commit that referenced this pull request Mar 23, 2023
@tokatoka
Copy link
Member

@SpaceWhite can you check this again?

@SpaceWhite
Copy link
Contributor Author

@tokatoka Sorry for the long wait. I just sent the PR to fix this.

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.

Changing testcase filename should rename the file on disk
4 participants