View Issue Details

Related ChangesetsJump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0000233bareos-corestorage daemonpublic2013-10-09 17:152015-03-25 19:18
Reporterbastian Assigned To 
PrioritynormalSeveritymajorReproducibilityhave not tried
Status closedResolutionfixed 
PlatformLinuxOSanyOS Version3
Product Version13.2.1 
Fixed in Version13.2.3 
Summary0000233: Missing handling of "close" errors
DescriptionPlease note: I have not (yet) reproduced the problem with bareos, but encountered it in Bacula. The respective code is unchanged, however.

When backing up on a CIFS target (others?), a full (remote) file system is not detected until close() returns (returning -1 and setting errno). However, the return value is not verified, and the incorrect write operation is not detected. The final job result is "ok", but the volume files are small or empty.
Steps To ReproduceConfigure a file based backup storage on a CIFS mounted share. Fill the remote file system until only a small amount of space is left. Run backup on that storage that contains more than that amount of data.

Results in an "OK" job and a full disk; no error messages whatsoever.
Additional InformationIn src/stored/dev.c, line 1735 (current git master head), "d_close(m_fd)" is called without checking its return value and/or errno.

I'd expect the attached patch to fix that problem. However, This Is Completely Untested[tm] to the extent that I did not even try to compile it!!!111einself

Thx to the brain-dead original project I'm not even able to fix the problem there, as Kern decided not to provide access to dcr in DEVICE::close. Thanks that you do :) One step closer to migrating.
TagsNo tags attached.

Activities

bastian

bastian

2013-10-09 17:15

reporter  

handle-close-errors.diff (543 bytes)    
diff --git a/src/stored/dev.c b/src/stored/dev.c
index 4fe1ca2..27144f0 100644
--- a/src/stored/dev.c
+++ b/src/stored/dev.c
@@ -1732,7 +1732,13 @@ void DEVICE::close(DCR *dcr)
       unlock_door();
       /* Fall through wanted */
    default:
-      d_close(m_fd);
+      int stat = d_close(m_fd);
+      if (stat < 0) {
+         berrno be;
+         Mmsg2(errmsg, _("Unable to close device %s. ERR=%s\n"), 
+                  print_name(), be.bstrerror());
+         Jmsg(dcr->jcr, M_FATAL, 0, dev->errmsg);
+      }
       break;
    }
handle-close-errors.diff (543 bytes)   
mvwieringen

mvwieringen

2013-10-09 17:40

developer   ~0000694

Yes indeed its a bug not to check the return code of close(). It can indeed
return ENOSPC (just checked). Your patch almost works. e.g. the dev-> can
go as we are in the device class so we can just use errmsg which is a member
of the class you also fill that already. Other then I hate using a variable
named stat (I think I mostly changed them to status as stat is also the name
of a function ...). I see some more close calls but have to look if we want
to fail the job there too but ignoring the close() return value is indeed
something you should worry about.

Given this is good "defensive" programming anyway I will push it a least into
master soon. And if we have given it some time to settle there we can
backport it. I will commit the patch under your name later tonight.
mvwieringen

mvwieringen

2013-10-09 17:55

developer   ~0000695

It might be we have to explicitly check for errno == ENOSPC then again the
return values from close are:

All from the Solaris man page but I think its the same on all UNIX like systems.

The close() function will fail if:

EBADF The fildes argument is not a valid file descriptor.
EINTR The close() function was interrupted by a signal.
ENOLINK The fildes argument is on a remote machine and
           the link to that machine is no longer active.
ENOSPC There was no free space remaining on the device
           containing the file.

The close() function may fail if:

EIO An I/O error occurred while reading from or writing
           to the file system.

If I look at the errors above I think only EBADF is something not so
fatal then again its a programming error. So lets see what this patch
brings in regression tonight.
bastian

bastian

2013-10-09 17:58

reporter   ~0000696

Fix committed to bareos master branch with changesetid 1208.
bastian

bastian

2013-10-14 19:19

reporter   ~0000701

Fix committed to bareos bareos-12.4 branch with changesetid 1211.
bastian

bastian

2013-10-14 19:19

reporter   ~0000702

Fix committed to bareos bareos-13.2 branch with changesetid 1214.
mvwieringen

mvwieringen

2015-03-25 16:51

developer   ~0001359

Fix committed to bareos2015 bareos-14.2 branch with changesetid 5014.
joergs

joergs

2015-03-25 19:18

developer   ~0001514

Due to the reimport of the Github repository to bugs.bareos.org, the status of some tickets have been changed. These tickets will be closed again.
Sorry for the noise.

Related Changesets

bareos: master d2296ef6

2013-10-09 17:43

bastian

Ported: N/A

Details Diff		 Missing handling of "close" errors

When backing up on a CIFS target (others?), a full (remote) file system
is not detected until close() returns (returning -1 and setting errno).
However, the return value is not verified, and the incorrect write
operation is not detected. The final job result is "ok", but the volume
files are small or empty.

Fixes 0000233: Missing handling of "close" errors

Signed-off-by: Marco van Wieringen <marco.van.wieringen@bareos.com>		 Affected Issues
0000233
mod - src/stored/dev.c Diff File

bareos: bareos-12.4 ce6baf19

2013-10-09 17:43

bastian

Ported: N/A

Details Diff		 Missing handling of "close" errors

When backing up on a CIFS target (others?), a full (remote) file system
is not detected until close() returns (returning -1 and setting errno).
However, the return value is not verified, and the incorrect write
operation is not detected. The final job result is "ok", but the volume
files are small or empty.

Fixes 0000233: Missing handling of "close" errors

Signed-off-by: Marco van Wieringen <marco.van.wieringen@bareos.com>		 Affected Issues
0000233
mod - src/stored/dev.c Diff File

bareos: bareos-13.2 b27b5445

2013-10-09 17:43

bastian

Ported: N/A

Details Diff		 Missing handling of "close" errors

When backing up on a CIFS target (others?), a full (remote) file system
is not detected until close() returns (returning -1 and setting errno).
However, the return value is not verified, and the incorrect write
operation is not detected. The final job result is "ok", but the volume
files are small or empty.

Fixes 0000233: Missing handling of "close" errors

Signed-off-by: Marco van Wieringen <marco.van.wieringen@bareos.com>		 Affected Issues
0000233
mod - src/stored/dev.c Diff File

bareos2015: bareos-12.4 6ab55b34

2013-10-09 19:43

bastian


Committer: mvwieringen

Ported: N/A

Details Diff		 Missing handling of "close" errors

When backing up on a CIFS target (others?), a full (remote) file system
is not detected until close() returns (returning -1 and setting errno).
However, the return value is not verified, and the incorrect write
operation is not detected. The final job result is "ok", but the volume
files are small or empty.

Fixes 0000233: Missing handling of "close" errors

Signed-off-by: Marco van Wieringen <marco.van.wieringen@bareos.com>		 Affected Issues
0000233
mod - src/stored/dev.c Diff File

bareos2015: bareos-13.2 56ea00de

2013-10-09 19:43

bastian


Committer: mvwieringen

Ported: N/A

Details Diff		 Missing handling of "close" errors

When backing up on a CIFS target (others?), a full (remote) file system
is not detected until close() returns (returning -1 and setting errno).
However, the return value is not verified, and the incorrect write
operation is not detected. The final job result is "ok", but the volume
files are small or empty.

Fixes 0000233: Missing handling of "close" errors

Signed-off-by: Marco van Wieringen <marco.van.wieringen@bareos.com>		 Affected Issues
0000233
mod - src/stored/dev.c Diff File

bareos2015: bareos-14.2 d32de662

2013-10-09 19:43

bastian


Committer: mvwieringen

Ported: N/A

Details Diff		 Missing handling of "close" errors

When backing up on a CIFS target (others?), a full (remote) file system
is not detected until close() returns (returning -1 and setting errno).
However, the return value is not verified, and the incorrect write
operation is not detected. The final job result is "ok", but the volume
files are small or empty.

Fixes 0000233: Missing handling of "close" errors

Signed-off-by: Marco van Wieringen <marco.van.wieringen@bareos.com>		 Affected Issues
0000233
mod - src/stored/dev.c Diff File

Issue History

Date Modified Username Field Change
2013-10-09 17:15 bastian New Issue
2013-10-09 17:15 bastian File Added: handle-close-errors.diff
2013-10-09 17:40 mvwieringen Note Added: 0000694
2013-10-09 17:40 mvwieringen Assigned To => mvwieringen
2013-10-09 17:40 mvwieringen Status new => assigned
2013-10-09 17:55 mvwieringen Note Added: 0000695
2013-10-09 17:58 bastian Changeset attached => bareos master d2296ef6
2013-10-09 17:58 bastian Note Added: 0000696
2013-10-09 17:58 bastian Assigned To mvwieringen => bastian
2013-10-09 17:58 bastian Status assigned => resolved
2013-10-09 17:58 bastian Resolution open => fixed
2013-10-14 19:19 bastian Changeset attached => bareos bareos-12.4 ce6baf19
2013-10-14 19:19 bastian Note Added: 0000701
2013-10-14 19:19 bastian Changeset attached => bareos bareos-13.2 b27b5445
2013-10-14 19:19 bastian Note Added: 0000702
2014-05-16 17:37 mvwieringen adm Assigned To bastian =>
2014-05-16 17:37 mvwieringen adm Status resolved => closed
2014-05-16 17:37 mvwieringen adm Fixed in Version => 13.2.3
2015-03-25 16:51 mvwieringen Changeset attached => bareos2015 bareos-12.4 6ab55b34
2015-03-25 16:51 mvwieringen Changeset attached => bareos2015 bareos-13.2 56ea00de
2015-03-25 16:51 mvwieringen Changeset attached => bareos2015 bareos-14.2 d32de662
2015-03-25 16:51 mvwieringen Note Added: 0001359
2015-03-25 16:51 mvwieringen Status closed => resolved
2015-03-25 19:18 joergs Note Added: 0001514
2015-03-25 19:18 joergs Status resolved => closed