Skip to content

delete_directory fails silently if it contains subfolders #637

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 2 commits into from
Mar 21, 2017

Conversation

NathanJPhillips
Copy link
Contributor

No description provided.

@tautschnig
Copy link
Collaborator

Would the _WIN32 version also need such a fix?

@@ -107,7 +107,13 @@ void delete_directory(const std::string &path)
struct dirent *ent;

while((ent=readdir(dir))!=NULL)
remove((path+"/"+ent->d_name).c_str());
{
std::string sub_path = path+"/"+ent->d_name;
Copy link
Member

Choose a reason for hiding this comment

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

sub_path=path...

remove((path+"/"+ent->d_name).c_str());
{
std::string sub_path = path+"/"+ent->d_name;
if(ent->d_type == DT_DIR)
Copy link
Member

Choose a reason for hiding this comment

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

d_type==DT_DIR

@@ -107,7 +107,13 @@ void delete_directory(const std::string &path)
struct dirent *ent;

while((ent=readdir(dir))!=NULL)
Copy link
Member

Choose a reason for hiding this comment

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

nullptr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not my code. I fear that if I change this it will expand the context area and you'll then find something else above that you want me to change too.

@NathanJPhillips
Copy link
Contributor Author

I've fixed @peterschrammel's lint errors. Unfortunately I don't have a Windows environment set up to implement and test this behaviour on Windows. I only use this fix in regression tests in the security scanner repo though so any issues will be quickly apparent and won't affect core CBMC.

@tautschnig
Copy link
Collaborator

I believe @kroening has a Windows environment (and there would be one available via AppVeyor).

@NathanJPhillips
Copy link
Contributor Author

Does @kroening want to test the following code to see if it can delete a folder containing a sub-folder and some files? If this is going to take much more of my time though I'm probably just going to close this issue and give up. Shame you can't get an improvement into CBMC without having to fix other broken code around it. Makes things take so long that really shouldn't.

  struct _finddata_t info;
  intptr_t hFile=_findfirst(pattern.c_str(), &info);
  if(hFile!=-1)
  {
    do {
      std::string sub_path=path+"\\"+info.name;
      if(info.attrib==_A_SUBDIR)
        delete_directory(sub_path);
      else
        unlink(sub_path);
    } while(_findnext(hFile, &info)==0)
    _findclose(hFile);
  }

@NathanJPhillips
Copy link
Contributor Author

Did the previous code even work on Windows? unlink seems to be called with a path relative to the passed folder.

@peterschrammel
Copy link
Member

I suggest to create a unit test for this in the /unit folder. So, we know at least if it works/worked/will-work-soon on all platforms.

@NathanJPhillips
Copy link
Contributor Author

NathanJPhillips commented Mar 21, 2017

Code written and tested by @smowton for Windows has been added.

@NathanJPhillips NathanJPhillips force-pushed the bugfix/file_util branch 3 times, most recently from a5caaf7 to 129a592 Compare March 21, 2017 10:18
@tautschnig
Copy link
Collaborator

Thanks @NathanJPhillips and @smowton!

@kroening kroening merged commit 6edd2b3 into diffblue:master Mar 21, 2017
@NathanJPhillips NathanJPhillips deleted the bugfix/file_util branch March 21, 2017 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants