Loading ...
Sorry, an error occurred while loading the content.

Bug in server-src/diskfile.c breaks multiple scripts per dle

Expand Messages
  • Ralph Rößner
    Hi! Using a dumptype with more than one script defined causes amcheck (and probably others) to fail, citing a format error in the request packet as the reason.
    Message 1 of 3 , Oct 7, 2009
    • 0 Attachment
      Hi!

      Using a dumptype with more than one script defined causes amcheck (and
      probably others) to fail, citing a format error in the request packet as
      the reason. This has been encountered in 2.6.1p1 and verified to persist
      in SVN revision 2162.


      * Example dumptype

      define dumptype normal-snapshot {
      normal
      script "create-lvm-snapshots"
      script "remove-lvm-snapshots"
      }


      * Example invocation:

      backup@keldon:~$ amcheck -c CAPCom

      Amanda Backup Client Hosts Check
      --------------------------------
      ERROR: keldon: [FORMAT ERROR IN REQUEST PACKET Error on line 1 char 459: Element 'dle' was closed, but the currently open element is 'script']
      ERROR: keldon: service /usr/lib/amanda/selfcheck failed: pid 10527 exited with code 1
      Client check: 14 hosts checked in 2.125 seconds. 2 problems found.

      (brought to you by Amanda 2.6.1p1)


      * Analysis

      Monitoring the communication shows that in the request packet the second
      script block is indeed missing its closing tag. The first script block is
      syntactically correct.

      Encoding of the script data is done in xml_scripts() in diskfile.c . The
      </script> closing tag is added in #1940 by call to vstrextend(), which
      treats its second to last argument as a variable argument list. A NULL
      pointer passed to vstrextend() thus terminates the argument list,
      regardless of any following non-NULL arguments.

      The last argument before the closing tag is xml_app.result, which is
      allocated in #1840, outside the for-all-scripts loop, but freed in #1942,
      inside the same loop. So for all passes through the loop after the first
      one, xml_app.result will be NULL and the closing tag will be ignored by
      vstrextend().

      Besides breaking the script encoding, other bad things might happen if
      code inside the loop assumes that xml_app.result is initialized.


      * Suggested Fix

      Initialize xml_app.result inside the loop body. Proposed patch (unidiff):

      --- diskfile.c.orig 2009-10-07 11:53:08.000000000 +0200
      +++ diskfile.c 2009-10-07 12:56:06.000000000 +0200
      @@ -1837,7 +1837,7 @@
      xml_app_t xml_app;

      xml_app.features = their_features;
      - xml_app.result = stralloc("");
      +/* xml_app.result initialized in loop */

      xml_scr = stralloc("");
      for (pp_iter = pp_scriptlist; pp_iter != NULL;
      @@ -1850,6 +1850,7 @@
      xml_scr1 = vstralloc(" <script>\n",
      " ", b64plugin, "\n",
      NULL);
      + xml_app.result = stralloc("");

      execute_where = pp_script_get_execute_where(pp_script);
      switch (execute_where) {


      A 2162 revision modified in this way does not reproduce the error.


      By the way, is posting to amanda-hackers the canonical way of reporting
      bugs in amanda? I could not find the bug tracker on the sourceforge page
      any more.

      Sincerely,
      Ralph Rößner

      --
      Ralph Rößner
      CAPCom AG < http://www.capcom.de >
      Lise - Meitner - Straße 10, 64293 Darmstadt, Deutschland
      Phone +49 6151 155 900, Fax +49 6151 155 909
      Mobil: +49 170 2212 411

      Vorstand: Luc Neumann (Vorsitzender)
      Vorsitzender des Aufsichtsrats: Prof. Dr.-Ing. José L. Encarnação
      Sitz der Gesellschaft: Darmstadt, Registergericht: Darmstadt HRB 8090
    • Jean-Louis Martineau
      Ralph, Your patch is good. Reporting to amanda-hackers is the best way to report bug and patch in amanda. Jean-Louis
      Message 2 of 3 , Oct 7, 2009
      • 0 Attachment
        Ralph,

        Your patch is good.
        Reporting to amanda-hackers is the best way to report bug and patch in
        amanda.

        Jean-Louis

        Ralph Rößner wrote:
        > Hi!
        >
        > Using a dumptype with more than one script defined causes amcheck (and
        > probably others) to fail, citing a format error in the request packet as
        > the reason. This has been encountered in 2.6.1p1 and verified to persist
        > in SVN revision 2162.
        >
        >
        > * Example dumptype
        >
        > define dumptype normal-snapshot {
        > normal
        > script "create-lvm-snapshots"
        > script "remove-lvm-snapshots"
        > }
        >
        >
        > * Example invocation:
        >
        > backup@keldon:~$ amcheck -c CAPCom
        >
        > Amanda Backup Client Hosts Check
        > --------------------------------
        > ERROR: keldon: [FORMAT ERROR IN REQUEST PACKET Error on line 1 char 459: Element 'dle' was closed, but the currently open element is 'script']
        > ERROR: keldon: service /usr/lib/amanda/selfcheck failed: pid 10527 exited with code 1
        > Client check: 14 hosts checked in 2.125 seconds. 2 problems found.
        >
        > (brought to you by Amanda 2.6.1p1)
        >
        >
        > * Analysis
        >
        > Monitoring the communication shows that in the request packet the second
        > script block is indeed missing its closing tag. The first script block is
        > syntactically correct.
        >
        > Encoding of the script data is done in xml_scripts() in diskfile.c . The
        > </script> closing tag is added in #1940 by call to vstrextend(), which
        > treats its second to last argument as a variable argument list. A NULL
        > pointer passed to vstrextend() thus terminates the argument list,
        > regardless of any following non-NULL arguments.
        >
        > The last argument before the closing tag is xml_app.result, which is
        > allocated in #1840, outside the for-all-scripts loop, but freed in #1942,
        > inside the same loop. So for all passes through the loop after the first
        > one, xml_app.result will be NULL and the closing tag will be ignored by
        > vstrextend().
        >
        > Besides breaking the script encoding, other bad things might happen if
        > code inside the loop assumes that xml_app.result is initialized.
        >
        >
        > * Suggested Fix
        >
        > Initialize xml_app.result inside the loop body. Proposed patch (unidiff):
        >
        > --- diskfile.c.orig 2009-10-07 11:53:08.000000000 +0200
        > +++ diskfile.c 2009-10-07 12:56:06.000000000 +0200
        > @@ -1837,7 +1837,7 @@
        > xml_app_t xml_app;
        >
        > xml_app.features = their_features;
        > - xml_app.result = stralloc("");
        > +/* xml_app.result initialized in loop */
        >
        > xml_scr = stralloc("");
        > for (pp_iter = pp_scriptlist; pp_iter != NULL;
        > @@ -1850,6 +1850,7 @@
        > xml_scr1 = vstralloc(" <script>\n",
        > " ", b64plugin, "\n",
        > NULL);
        > + xml_app.result = stralloc("");
        >
        > execute_where = pp_script_get_execute_where(pp_script);
        > switch (execute_where) {
        >
        >
        > A 2162 revision modified in this way does not reproduce the error.
        >
        >
        > By the way, is posting to amanda-hackers the canonical way of reporting
        > bugs in amanda? I could not find the bug tracker on the sourceforge page
        > any more.
        >
        > Sincerely,
        > Ralph Rößne
      • Dustin J. Mitchell
        ... Ralph -- thanks for the submission! Yes, this is the proper venue for bugs and patches. The patch looks fine to me, so as long as Jean-Louis approves, I
        Message 3 of 3 , Oct 7, 2009
        • 0 Attachment
          On Wed, Oct 7, 2009 at 8:33 AM, Ralph Rößner <roessner@...> wrote:
          > By the way, is posting to amanda-hackers the canonical way of reporting
          > bugs in amanda? I could not find the bug tracker on the sourceforge page
          > any more.

          Ralph -- thanks for the submission! Yes, this is the proper venue for
          bugs and patches.

          The patch looks fine to me, so as long as Jean-Louis approves, I think
          this is good to go. Jean-Louis will commit it.

          Please let us know if you spot any other glaring errors!

          Dustin

          --
          Open Source Storage Engineer
          http://www.zmanda.com
        Your message has been successfully submitted and would be delivered to recipients shortly.