[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Silent reverting of my changes to libdebian-installer

In libdebian-installer 0.60, I made the following change (r55491):

  * Appease the combination of _FORTIFY_SOURCE=2 (used by default on Ubuntu)
    and -Werror. Why exactly glibc demands that fwrite be checked but not
    fputs is beyond me.

In libdebian-installer 0.61, Bastian Blank made the following change

  * Remove workarounds for ignored errors.

... which simply reverts my change. Because as far as I can tell nobody
had the basic courtesy to mail me about this, I didn't notice until

Funnily enough, I didn't make that change to annoy people; I made it
because I needed it, and I committed it to upstream d-i because it was
harmless and there was no point in introducing divergence between Debian
and Ubuntu for something harmless to Debian. (People criticise me enough
for the cases that *are* currently necessary that I'd rather keep
divergence to a minimum, which I thought was a goal everyone could
happily agree with and certainly one I am currently working towards; in
this case the change seemed uncomplicated and it was my judgement that
it was unlikely to cause offence, so I didn't feel I needed to waste
people's time by mailing it to the list. Furthermore I'm in libd-i's
Uploaders and as far as I know nobody has objected to that!)

Bastian's claim that the error is ignored is wrong in one of the two
cases; my pipe change does not ignore the error.

I think my change should be reinstated because:

  * Even though my fwrite change has no functional effect, it documents
    that the error is known and ignored for the time being, which is
    worthwhile and generally good practice. It happens to do so in such
    a way as to appease -D_FORTIFY_SOURCE=2 -Werror, which I don't think
    is harmful (note that this is not the thin end of the wedge; this
    set of libd-i changes was the only set of changes I had to make to
    all of d-i for this, although that may well be because -Werror is
    not used universally). I can probably arrange to wrap it in a macro
    for readability if you'd prefer that.

    Note that this is a real and not insignificant bug; if you run out
    of memory, di_parser_rfc822_write_file may silently truncate the
    file it's writing. This is surely worth a FIXME comment or two even
    if they aren't pretty.

  * My pipe change in internal_di_exec has a real functional effect: it
    causes internal_di_exec to fail if pipe fails, rather than merrily
    carrying on as if nothing had happened. The code as reverted is
    simply wrong even if the compiler flags in use by default in Debian
    don't notice it. pipe can fail for slightly unlikely but quite
    possible reasons (e.g. running out of per-process file descriptors),
    and in the event that that happens then internal_di_exec shouldn't
    just carry on.

Perhaps I should have written more about this in my changelog comment,
but I thought it would be obvious that compiler warnings (converted to
errors by -Werror) often indicate real bugs.

I'm quite happy to have a civilised discussion about these changes;
circumstances beyond my control force me to carry some kind of fix for
-D_FORTIFY_SOURCE=2 -Werror compilation in Ubuntu regardless, but that
doesn't mean that their form is fixed. I really object to this practice
of backing out changes without comment though.

The patch content of r55491 is attached to this mail.


Colin Watson                                       [cjwatson@debian.org]
Index: debian/changelog
--- debian/changelog	(revision 55490)
+++ debian/changelog	(revision 55491)
@@ -2,6 +2,9 @@ libdebian-installer (0.60) UNRELEASED; urgency=low
   * Remove incorrect unused attribute from di_packages_parser_read_name's
     user_data parameter.
+  * Appease the combination of _FORTIFY_SOURCE=2 (used by default on Ubuntu)
+    and -Werror. Why exactly glibc demands that fwrite be checked but not
+    fputs is beyond me.
  -- Colin Watson <cjwatson@debian.org>  Mon, 01 Sep 2008 23:50:02 +0100
Index: src/parser_rfc822.c
--- src/parser_rfc822.c	(revision 55490)
+++ src/parser_rfc822.c	(revision 55491)
@@ -244,9 +244,15 @@ cleanup:
 static void callback (const di_rstring *field, const di_rstring *value, void *data)
   FILE *f = data;
-  fwrite (field->string, field->size, 1, f);
+  if (fwrite (field->string, field->size, 1, f) < 1)
+  {
+    /* FIXME: can't handle write errors, but appease _FORTIFY_SOURCE=2 */
+  }
   fputs (": ", f);
-  fwrite (value->string, value->size, 1, f);
+  if (fwrite (value->string, value->size, 1, f) < 1)
+  {
+    /* FIXME: can't handle write errors, but appease _FORTIFY_SOURCE=2 */
+  }
   fputs ("\n", f);
Index: src/exec.c
--- src/exec.c	(revision 55490)
+++ src/exec.c	(revision 55491)
@@ -60,7 +60,19 @@ static int internal_di_exec (const char *path, boo
   for (i = 0; i < pipes; i++)
-    pipe (&fds[i * 2]);
+  {
+    if (pipe (&fds[i * 2]) < 0)
+    {
+      int j;
+      di_log (DI_LOG_LEVEL_WARNING, "pipe failed");
+      for (j = 0; j < i; j++)
+      {
+        close (fds[i * 2]);
+        close (fds[i * 2 + 1]);
+      }
+      return -1;
+    }
+  }
   pid = fork ();

Reply to: