GNOME Bugzilla – Bug 338196
Use after free in xml-dir.c
Last modified: 2006-05-06 11:32:01 UTC
Coverity report #1239: Event freed_arg: Pointer "outfile" freed by function "fclose" Also see events: [double_free] At conditional (1): "fclose < 0" taking true path 504 if (fclose (outfile) < 0) 505 { 506 gconf_set_error (err, GCONF_ERROR_FAILED, 507 _("Failed to close file `%s': %s"), 508 tmp_filename, g_strerror (errno)); 509 510 retval = FALSE; 511 goto failed_end_of_sync; 512 } 513 514 outfile = NULL; 515 516 #ifndef HAVE_FCHMOD 517 /* Set permissions on the new file */ 518 if (chmod (tmp_filename, d->file_mode) != 0) 519 { 520 gconf_set_error(err, GCONF_ERROR_FAILED, 521 _("Failed to set mode on `%s': %s"), 522 tmp_filename, g_strerror(errno)); 523 524 retval = FALSE; 525 goto failed_end_of_sync; 526 } 527 #endif 528 529 old_existed = gconf_file_exists (d->xml_filename); 530 531 if (old_existed) 532 { 533 if (g_rename(d->xml_filename, old_filename) < 0) 534 { 535 gconf_set_error(err, GCONF_ERROR_FAILED, 536 _("Failed to rename `%s' to `%s': %s"), 537 d->xml_filename, old_filename, g_strerror(errno)); 538 539 retval = FALSE; 540 goto failed_end_of_sync; 541 } 542 } 543 544 if (g_rename(tmp_filename, d->xml_filename) < 0) 545 { 546 gconf_set_error(err, GCONF_ERROR_FAILED, _("Failed to rename `%s' to `%s': %s"), 547 tmp_filename, d->xml_filename, g_strerror(errno)); 548 549 /* Put the original file back, so this isn't a total disaster. */ 550 if (g_rename(old_filename, d->xml_filename) < 0) 551 { 552 gconf_set_error(err, GCONF_ERROR_FAILED, _("Failed to restore `%s' from `%s': %s"), 553 d->xml_filename, old_filename, g_strerror(errno)); 554 } 555 556 retval = FALSE; 557 goto failed_end_of_sync; 558 } 559 560 if (old_existed) 561 { 562 if (g_unlink(old_filename) < 0) 563 { 564 gconf_log(GCL_WARNING, _("Failed to delete old file `%s': %s"), 565 old_filename, g_strerror(errno)); 566 /* Not a failure, just leaves cruft around. */ 567 } 568 } 569 570 failed_end_of_sync: 571 572 g_free(old_filename); 573 g_free(tmp_filename); At conditional (2): "outfile != 0" taking true path 574 if (outfile) Event double_free: Double free of pointer "outfile" in call to "fclose" Also see events: [freed_arg] 575 fclose (outfile); 576 } 577 578 if (retval) 579 d->dirty = FALSE; 580 581 return retval; 582 } Attaching a proposed fix.
Created attachment 63298 [details] [review] proposed patch
Doesn't look right ... your patch makes us set outfile = NULL where we're already guaranteed it's NULL
How are we guaranteed that it's NULL? If fclose succeeds it's set to NULL right after the test, but if it fails any further access to the stream results in undefined behaviour according to the man page. (or my understanding of it at least). Would just deleting the last test and fclose work?
Apologies, I was looking in the wrong place
But is the patch correct or is even setting it to NULL giving undefined behaviour so we should just remove all references to it after the fclose call?
Commited. We'll see if that shuts up the checker.