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

Don't allow invalid project files! #2523

Merged
merged 1 commit into from Sep 5, 2016

Conversation

@zonkmachine
Copy link
Member

zonkmachine commented Jan 28, 2016

The renderer hangs bad if you give it something that isn't an LMMS project as argument.


Loading project...
Done
|-                                 |      0%   \   <---infinite hang here!

This fix tests that the given file is really a file ( not a directory ). It also tests for a valid LMMS project suffix.

~/builds/lmms/build$ ./lmms -r ../build
The file ../build does not exist!

You can test specifically for if the argument is a directory and report accordingly but that would be over doing it a bit I think.

~/builds/lmms/build$ ./lmms -r ../build/lmmscon
The file ../build/lmmscon does not exist!
~/builds/lmms/build$ ./lmms -r ../build/lmmsconfig.h 
../build/lmmsconfig.h is not an LMMS project fil

I finally moved the line Engine::init( true ); to after the tests as it just felt like a better place.

@zonkmachine zonkmachine added the bug label Jan 28, 2016
@zonkmachine zonkmachine added this to the 1.2.0 milestone Jan 28, 2016
@zonkmachine zonkmachine changed the title Don't allow crap as render argument! Don't allow invalid arguments to the renderer! Jan 28, 2016
@tresf
Copy link
Member

tresf commented Jan 28, 2016

Looks good. 👍 I'll leave time for comments. One thought was the posiblity of loading a non-mmp or non-mmpz file, but loadFile only takes projects, so that's for another time. 👍

@zonkmachine
Copy link
Member Author

zonkmachine commented Jan 28, 2016

One thought was the posiblity of loading a non-mmp or non-mmpz file,

Yes, I was going to ask about it.
I'm currently trying to move the whole file test to here. Right after fileToLoad is assigned a file.
We're currently treating file arguments for render and load differently and this should fix it. It works but I haven't tested it thoroughly. It probably makes the larger part of #2524 redundant

@zonkmachine
Copy link
Member Author

zonkmachine commented Jan 29, 2016

In the last version it still tests for an empty fileToLoad if you choose --import and halts with a
The file does not exist!

@zonkmachine zonkmachine changed the title Don't allow invalid arguments to the renderer! Don't allow invalid project file arguments! Jan 29, 2016
@zonkmachine zonkmachine changed the title Don't allow invalid project file arguments! Don't allow invalid project files! Jan 29, 2016
@zonkmachine
Copy link
Member Author

zonkmachine commented Jan 29, 2016

importsuffixes

Same checks for files to import.

@zonkmachine zonkmachine force-pushed the zonkmachine:directoryargument branch from 2e8aaaa to 469117d Jan 29, 2016
@zonkmachine
Copy link
Member Author

zonkmachine commented Jan 29, 2016

One thought was the posiblity of loading a non-mmp or non-mmpz file

An mpt file?

@softrabbit
Copy link
Member

softrabbit commented Jan 30, 2016

An mpt file?

Any valid LMMS project file that happens to be named differently from what's expected? If I explicitly tell LMMS to load a file called "myproject.mmpx" or whatever, I expect it to at least try and not reject the file until it's at least opened the file and tried parsing it.

@zonkmachine
Copy link
Member Author

zonkmachine commented Feb 7, 2016

Any valid LMMS project file that happens to be named differently from what's expected? If I explicitly tell LMMS to load a file called "myproject.mmpx" or whatever, I expect it to at least try and not reject the file until it's at least opened the file and tried parsing it.

OK. Right now if you try and save a project file with another suffix than .mmp or .mmpz, it will stick a new suffix after the one you opted for. In your case with myproject.mmpx you will get myproject.mmpx.mmpz so on my Ubuntu/Mint I have to go back and change suffix manually and then myproject.mmpx will open as you expect it. Either as a command line argument or from the gui.

Edit: Only files ending mmp/mmpz will show up in the file open dialogue so other valid projects will have to be loaded from the command line.

@zonkmachine zonkmachine force-pushed the zonkmachine:directoryargument branch from 469117d to 9600c50 Feb 13, 2016
fileInfoImport.suffix().toLower() == "midi" ||
fileInfoImport.suffix().toLower() == "rmi" ||
fileInfoImport.suffix().toLower() == "flp" ||
fileInfoImport.suffix().toLower() == "h2song" ) )

This comment has been minimized.

@tresf

tresf Apr 23, 2016 Member

Hmm.... Every time I see a laundry list of file extensions I get worried that we're making more work for ourselves down the road. 😄 Perhaps related to this PR, perhaps not.... can we start managing our extension in a centralized place? We should be able to use #ifdef's to enable/disable them globally based on what libraries/plugins cmake finds. We already do this in several other places already. Food for thought.

This comment has been minimized.

@zonkmachine

zonkmachine May 5, 2016 Author Member

Perhaps related to this PR, perhaps not.... can we start managing our extension in a centralized place?

Related but it doesn't sound like something I can implement.

// Test file argument before continuing
QFileInfo fileInfoLoad( fileToLoad );
QFileInfo fileInfoImport( fileToImport );
if( !fileToLoad.isEmpty() )

This comment has been minimized.

@tresf

tresf Apr 23, 2016 Member

Shouldn't this be the other way around? Shouldn't we only instantiate fileInfoLoad if the QString isn't null or empty? This would also remove the need for setting an empty QString above.

This comment has been minimized.

@tresf

tresf Jul 5, 2016 Member

The above code comment from April still remains unanswered and you have a commented line that should be removed prior to merging.

This comment has been minimized.

@zonkmachine

zonkmachine Jul 18, 2016 Author Member

Shouldn't we only instantiate fileInfoLoad if the QString isn't null or empty? This would also remove the need for setting an empty QString above.

No, I don't think so, but this part is essentially the same as before. I just moved it and expanded on the functionality.

you have a commented line that should be removed

comment removed

This comment has been minimized.

@jasp00

jasp00 Jul 18, 2016 Member

fileInfoLoad should be instantiated in the if( !fileToLoad.isEmpty() ) block; fileInfoImport, in the if( !fileToImport.isEmpty() ) one.

This comment has been minimized.

@zonkmachine

zonkmachine Jul 19, 2016 Author Member

OK. I updated the according to Javiers suggestions.

@zonkmachine zonkmachine force-pushed the zonkmachine:directoryargument branch from 9600c50 to 4a3fca6 May 5, 2016
@zonkmachine
Copy link
Member Author

zonkmachine commented May 5, 2016

Conflicts resolved.

@zonkmachine zonkmachine force-pushed the zonkmachine:directoryargument branch from 4a3fca6 to 080119c May 5, 2016
@Umcaruje
Copy link
Member

Umcaruje commented Jul 8, 2016

@zonkmachine is there any more work needed to be done here, or it could be tested and merged?

@zonkmachine
Copy link
Member Author

zonkmachine commented Jul 8, 2016

Yes, mad sorry about the slow response here. Short on time. I'll look into it as soon as I possibly can.

@zonkmachine
Copy link
Member Author

zonkmachine commented Jul 10, 2016

OK. Trying to finish this one now.

@zonkmachine zonkmachine force-pushed the zonkmachine:directoryargument branch 2 times, most recently from 94d106a to 2bcda0e Jul 18, 2016
}
else if( !( fileInfoLoad.suffix().toLower() == "mmp" ||
fileInfoLoad.suffix().toLower() == "mmpz" ||
fileInfoLoad.suffix().toLower() == "mpt" ) )

This comment has been minimized.

@jasp00

jasp00 Jul 18, 2016 Member

Older projects use the xml extension.

This comment has been minimized.

@zonkmachine

zonkmachine Jul 19, 2016 Author Member

Fixed

else if( !( fileInfoImport.suffix().toLower() == "mid" ||
fileInfoImport.suffix().toLower() == "midi" ||
fileInfoImport.suffix().toLower() == "rmi" ||
fileInfoImport.suffix().toLower() == "flp" ||

This comment has been minimized.

@jasp00

jasp00 Jul 18, 2016 Member

FLP is dropped.

This comment has been minimized.

@zonkmachine

zonkmachine Jul 19, 2016 Author Member

Fixed. Should it also drop a message that FruityLoop support is dropped since version 1.2 ?

This comment has been minimized.

@jasp00

jasp00 Jul 19, 2016 Member

Such a message is useful before dropping support, when the feature is deprecated. It is not needed now.

This comment has been minimized.

@zonkmachine

zonkmachine Jul 19, 2016 Author Member

Aight!

@jasp00
Copy link
Member

jasp00 commented Jul 21, 2016

Current fix is incomplete. Input file could be an empty file, the empty template, a broken project or a different XML file. I do not think these new checks are necessary. The check for empty projects on export should be enough.

@jasp00
Copy link
Member

jasp00 commented Jul 21, 2016

I do not think these new checks are necessary.

I mean checking the extension, if the input does not exists, etc.

@zonkmachine
Copy link
Member Author

zonkmachine commented Aug 9, 2016

Right, back on this...

Input file could be an empty file,

Hm, missed that. Poking around with it right now...

the empty template

...Will open correctly.

a broken project

A file that is empty or a broken project will be passed to LMMS who will fail to open it and report on it.

emptyproject

It will then open an empty project with the title of the project you tried to open.

@zonkmachine zonkmachine force-pushed the zonkmachine:directoryargument branch from fcdf23a to b437266 Aug 9, 2016
@jasp00
Copy link
Member

jasp00 commented Aug 9, 2016

I mean that rendering those invalid projects (empty file, empty template, etc.) with lmms -r causes the infinite hang.

@zonkmachine
Copy link
Member Author

zonkmachine commented Aug 9, 2016

Right. In the case of an empty file the latest commit proposed a fix and it's working. An empty template however, I think goes a bit beyond this PR. Shouldn't it rather be catched by the renderer?

@zonkmachine
Copy link
Member Author

zonkmachine commented Aug 11, 2016

When exporting from the gui lmms ran TrackContainer::isEmpty() so I added this test in main.


$ ./lmms --rendertracks empty.mmpz
Notice: could not set realtime priority.
VST sync support disabled in your configuration
Loading project...
The project empty.mmpz is empty, aborting!

else if( !( fileInfoLoad.suffix().toLower() == "mmp" ||
fileInfoLoad.suffix().toLower() == "mmpz" ||
fileInfoLoad.suffix().toLower() == "mpt" ||
fileInfoLoad.suffix().toLower() == "xml" ) )

This comment has been minimized.

@jasp00

jasp00 Aug 17, 2016 Member

Having one of these extensions is not required; this check should be removed. The Engine::getSong()->isEmpty() check takes care of invalid projects.

This comment has been minimized.

@zonkmachine

zonkmachine Sep 1, 2016 Author Member

OK. I initially made these suffix tests because lmms opens in an unclean manner when you throw a file whatever at it. It will open a blank project with the title you just gave it. I think at least we should not set the project title when the project couldn't be opened.

This comment has been minimized.

@zonkmachine

zonkmachine Sep 1, 2016 Author Member

It will open a blank project with the title you just gave it. I think at least we should not set the project title when the project couldn't be opened.

That's a regression from 1.1.3 which does this just fine...

else if( !( fileInfoImport.suffix().toLower() == "mid" ||
fileInfoImport.suffix().toLower() == "midi" ||
fileInfoImport.suffix().toLower() == "rmi" ||
fileInfoImport.suffix().toLower() == "h2song" ) )

This comment has been minimized.

@jasp00

jasp00 Aug 17, 2016 Member

The import plug-ins know better how to handle these files; this check should be removed. ImportFilter::import() takes care of this.

if( !fileToLoad.isEmpty() )
{
QFileInfo fileInfoLoad( fileToLoad );
if( !fileInfoLoad.exists() )

This comment has been minimized.

@jasp00

jasp00 Aug 17, 2016 Member

!fileInfoLoad.size() checks if the file does not exist or its size is zero. The error message could be The file %s does not have any content.

This comment has been minimized.

@zonkmachine

zonkmachine Sep 1, 2016 Author Member

Fixed!

else if( !fileToImport.isEmpty() )
{
QFileInfo fileInfoImport( fileToImport );
if( !fileInfoImport.exists() )

This comment has been minimized.

@jasp00

jasp00 Aug 17, 2016 Member

!fileInfoImport.size() could be done here.

This comment has been minimized.

@zonkmachine

zonkmachine Sep 1, 2016 Author Member

Fixed!

printf( "%s is a directory.\n",
fileToImport.toStdString().c_str() );
exit( 1 );
}

This comment has been minimized.

@jasp00

jasp00 Aug 17, 2016 Member

These two tests (size() and isDir()) should be in a function.

This comment has been minimized.

@zonkmachine

zonkmachine Sep 1, 2016 Author Member

Aight! That cleaned things up. The PR shrunk by 50%... :)

@zonkmachine zonkmachine force-pushed the zonkmachine:directoryargument branch 2 times, most recently from 9e0cccc to d06f253 Sep 1, 2016
{
fileCheck( fileToLoad );
}
if( !fileToImport.isEmpty() )

This comment has been minimized.

@zonkmachine

zonkmachine Sep 1, 2016 Author Member

else if

Should I short this one out and give fileToLoad priority?

This comment has been minimized.

@zonkmachine

zonkmachine Sep 1, 2016 Author Member

Yeah, fixed that...

@zonkmachine zonkmachine force-pushed the zonkmachine:directoryargument branch from d06f253 to 5e5c88a Sep 1, 2016
@zonkmachine zonkmachine force-pushed the zonkmachine:directoryargument branch from 5e5c88a to 0039460 Sep 1, 2016
{
printf("The project %s is empty, aborting!\n", fileToLoad.toUtf8().constData() );
exit( EXIT_FAILURE );
}

This comment has been minimized.

@zonkmachine

zonkmachine Sep 1, 2016 Author Member

Is exit() even a good way to terminate the application at this point?

This comment has been minimized.

@jasp00

jasp00 Sep 4, 2016 Member

You may omit exit() and avoid the export, so an upgraded .lmmsrc.xml can be written, but exit() (or return) is not a bad way.

This comment has been minimized.

@zonkmachine

zonkmachine Sep 5, 2016 Author Member

oK. I just skimmed past some comment on the net that exit() could be a problem with multi threaded applications. I'm happy with .lmmsrc.xml not being written so we keep failing loads from appearing in the recently opened projects list.

@zonkmachine
Copy link
Member Author

zonkmachine commented Sep 2, 2016

lmms used to load default.mmp on failing to load a project but this was broken in commit: 2c7036e#diff-1c0d925f7437a8e3a837c0a991b4cf34

I'll try and look into the original issue (Edit: #781) behind that PR

Edit2: Fix for this in #3013

@zonkmachine
Copy link
Member Author

zonkmachine commented Sep 2, 2016

It's better to not inflate this PR. The regression in Song.cpp is not directly related to these fixes so I'll treat them separately.

Rebased and squashed! 🚜

@jasp00
Copy link
Member

jasp00 commented Sep 4, 2016

This request may be merged.

@zonkmachine zonkmachine merged commit a53fd26 into LMMS:master Sep 5, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zonkmachine
Copy link
Member Author

zonkmachine commented Sep 5, 2016

🚜

@zonkmachine zonkmachine deleted the zonkmachine:directoryargument branch Sep 5, 2016
sambler pushed a commit to sambler/lmms that referenced this pull request Sep 19, 2016
* master: (213 commits)
  Update Pattern and AutomationPattern length (LMMS#3037)
  Refresh i18n strings
  Hint text update
  Drop notes with length zero (LMMS#3031)
  Background tweak
  Background
  Update Flanger
  Exclude .ts files from the Github linguist
  Redesign Multitap echo (LMMS#3008)
  Update i18n source strings
  Extended arpeggiator functions (LMMS#2130)
  Fix sample track playback in BB tracks (LMMS#3023)
  Sort plug-in embedded resources (LMMS#3014)
  Implement version major.minor.release-stage.build (LMMS#3011)
  Fix regressions on loading broken projects (LMMS#3013)
  Improved file input validation. (LMMS#2523)
  Fix sample track view in BB editor (LMMS#3002)
  Request change in model when dropping a track (LMMS#3000)
  Add LocklessAllocator and use it in LocklessList (LMMS#2998)
  Drop forceStep in AutomatableModel (LMMS#3010)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.