Design flaw in Backup.pm

[alexskysilk]

it appears that there is a fault in the job validation code in backup.pm. The offending code is:

if ($job->{starttime} =~ m/^(\d{1,2}):(\d{1,2})$/) {
($hour, $minute) = (int($1), int($2));
die "hour '$hour' out of range\n" if $hour < 0 || $hour > 23;
die "minute '$minute' out of range\n" if $minute < 0 || $minute > 59;

I havent dug into the rest but it appears that this pulls ALL hour:minute values from vzdump.cron instead of just validating the job argument time. The end result is that if there is an offending (invalid) entry in vzdump.cron the check trips and ANY new job creation fails; this is obviously not the condition being tested.

This is important because failing to stagger jobs can cause the backup target storage to underperform when there are many jobs scheduled at the same time. Since there is no bulk stagger mechanism except to manually edit vzdump.cron, a wrong keystroke can render the whole mechanism nonfunctional...

 

[fabian]

it appears that there is a fault in the job validation code in backup.pm. The offending code is:

if ($job->{starttime} =~ m/^(\d{1,2}):(\d{1,2})$/) {
($hour, $minute) = (int($1), int($2));
die "hour '$hour' out of range\n" if $hour < 0 || $hour > 23;
die "minute '$minute' out of range\n" if $minute < 0 || $minute > 59;

I havent dug into the rest but it appears that this pulls ALL hour:minute values from vzdump.cron instead of just validating the job argument time. The end result is that if there is an offending (invalid) entry in vzdump.cron the check trips and ANY new job creation fails; this is obviously not the condition being tested.

This is important because failing to stagger jobs can cause the backup target storage to underperform when there are many jobs scheduled at the same time. Since there is no bulk stagger mechanism except to manually edit vzdump.cron, a wrong keystroke can render the whole mechanism nonfunctional...

the code is correct - it's the writer for an updated crontab file, and instead of writing an incomplete or broken crontab it should abort (and preserve the old - correct from PVE's view - version). if you manually break a file, you cannot expect PVE to unbreak it magically (it might in some cases )..

 

[alexskysilk]

Ok, I can accept that. In that case, would it not make sense to have separate function to add jobs and to validate the crontab?

On the other hand, this is an odd edge case... forget it