Patchwork argp: argp.doc prints incorrectly when it starts with "\v" [BZ #19038]

login
register
mail settings
Submitter Girish Joshi
Date Nov. 25, 2019, 6:08 p.m.
Message ID <CALkY8p9uj2VsOVsxvuyAejPVqhUbH14mYFRn=SDgJj0PrQZQow@mail.gmail.com>
Download mbox | patch
Permalink /patch/36191/
State New
Headers show

Comments

Girish Joshi - Nov. 25, 2019, 6:08 p.m.
Thanks Joseph for the review.
Here is a patch with the required changes.
Please let me know if more changes are needed in it.

argp/argp-help.c: added validation for leading \v in the doc string.
argp/tst-argp3.c: added test case when the doc string contains leading \v.
argp/Makefile: added tst-argp3 to tests

Overview:
argp.doc prints incorrectly when it starts with '\v'.
In argp-help.c in the function  variable  is being
initialized only once, which causes the whole documentation to be printed if
there is a leading '\v' in the doc string.

Implementation:
It needs to be initialized for every child in the doc and it needs to be printed
only in two cases.
1. There are no children and the doc does not starts with '\v'.
2. Argument  to the function  is false and the
complete doc needs to be printed.
---
 argp/Makefile    |  2 +-
 argp/argp-help.c | 10 +++++--
 argp/tst-argp3.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+), 3 deletions(-)
 create mode 100644 argp/tst-argp3.c
Girish Joshi - Nov. 30, 2019, 2:47 p.m.
Could anyone please review this patch?
Thanks.

Girish Joshi
girishjoshi.io

On Mon, Nov 25, 2019 at 11:38 PM Girish Joshi <girish946@gmail.com> wrote:
>
> Thanks Joseph for the review.
> Here is a patch with the required changes.
> Please let me know if more changes are needed in it.
>
> argp/argp-help.c: added validation for leading \v in the doc string.
> argp/tst-argp3.c: added test case when the doc string contains leading \v.
> argp/Makefile: added tst-argp3 to tests
>
> Overview:
> argp.doc prints incorrectly when it starts with '\v'.
> In argp-help.c in the function  variable  is being
> initialized only once, which causes the whole documentation to be printed if
> there is a leading '\v' in the doc string.
>
> Implementation:
> It needs to be initialized for every child in the doc and it needs to be printed
> only in two cases.
> 1. There are no children and the doc does not starts with '\v'.
> 2. Argument  to the function  is false and the
> complete doc needs to be printed.
> ---
>  argp/Makefile    |  2 +-
>  argp/argp-help.c | 10 +++++--
>  argp/tst-argp3.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 77 insertions(+), 3 deletions(-)
>  create mode 100644 argp/tst-argp3.c
>
> diff --git a/argp/Makefile b/argp/Makefile
> index c97e4c307c..8206da8cd8 100644
> --- a/argp/Makefile
> +++ b/argp/Makefile
> @@ -27,7 +27,7 @@ routines    = $(addprefix argp-, ba fmtstream
> fs-xinl help parse pv \
>                       pvh xinl eexst)
>
>  tests        = argp-test tst-argp1 bug-argp1 tst-argp2 bug-argp2 \
> -          tst-ldbl-argp
> +          tst-ldbl-argp tst-argp3
>
>  CFLAGS-argp-help.c += $(uses-callbacks) -fexceptions
>  CFLAGS-argp-parse.c += $(uses-callbacks)
> diff --git a/argp/argp-help.c b/argp/argp-help.c
> index 85f5792bfe..68422127d2 100644
> --- a/argp/argp-help.c
> +++ b/argp/argp-help.c
> @@ -1465,10 +1465,11 @@ argp_doc (const struct argp *argp, const
> struct argp_state *state,
>    size_t inp_text_limit = 0;
>    const char *doc = dgettext (argp->argp_domain, argp->doc);
>    const struct argp_child *child = argp->children;
> +  char *vt = 0;
>
>    if (doc)
>      {
> -      char *vt = strchr (doc, '\v');
> +      vt = strchr (doc, '\v');
>        inp_text = post ? (vt ? vt + 1 : 0) : doc;
>        inp_text_limit = (!post && vt) ? (vt - doc) : 0;
>      }
> @@ -1499,7 +1500,12 @@ argp_doc (const struct argp *argp, const struct
> argp_state *state,
>        if (text == inp_text && inp_text_limit)
>      __argp_fmtstream_write (stream, inp_text, inp_text_limit);
>        else
> -    __argp_fmtstream_puts (stream, text);
> +      {
> +        if ((!vt && !child) || (text == inp_text && !first_only))
> +        {
> +          __argp_fmtstream_puts (stream, text);
> +        }
> +      }
>
>        if (__argp_fmtstream_point (stream) > __argp_fmtstream_lmargin (stream))
>      __argp_fmtstream_putc (stream, '\n');
> diff --git a/argp/tst-argp3.c b/argp/tst-argp3.c
> new file mode 100644
> index 0000000000..997072ba7d
> --- /dev/null
> +++ b/argp/tst-argp3.c
> @@ -0,0 +1,68 @@
> +/* Test for argparse with leading '\v' in the doc string.
> +  Copyright (C) 2019 Free Software Foundation, Inc.
> +
> +  This program is free software: you can redistribute it and/or modify
> +  it under the terms of the GNU General Public License as published by
> +  the Free Software Foundation; either version 3 of the License, or
> +  (at your option) any later version.
> +
> +  This program is distributed in the hope that it will be useful,
> +  but WITHOUT ANY WARRANTY; without even the implied warranty of
> +  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +  GNU General Public License for more details.
> +
> +  You should have received a copy of the GNU General Public License
> +  along with this program.  If not, see <https://www.gnu.org/licenses/>.*/
> +
> +
> +#include<stdlib.h>
> +#include<argp.h>
> +
> +#include <support/capture_subprocess.h>
> +#include <support/check.h>
> +
> +
> +static char expected_success[] = "Usage: arp [OPTION...]\n\
> +\n\
> +  -?, --help                 Give this help list\n\
> +      --usage                Give a short usage message\n\
> +\n\
> +this is post_doc\n\
> +";
> +char *argv[3] = { (char *) "arp", NULL, NULL };
> +
> +static void
> +do_test_call (void)
> +{
> +  static char doc[] = "\vthis is post_doc";
> +  static struct argp argp = {NULL, NULL, NULL, doc};
> +
> +  argp_parse (&argp, 2, argv, 0, 0, NULL);
> +}
> +
> +static int
> +do_one_test (const char *expected)
> +{
> +  struct support_capture_subprocess result;
> +  result = support_capture_subprocess ((void *) &do_test_call, NULL);
> +
> +  TEST_COMPARE_STRING (result.out.buffer, expected);
> +
> +  return 0;
> +}
> +
> +
> +static int
> +do_test (void)
> +{
> +  const char *argument = "--help";
> +  argv[1] = (char *)argument;
> +  // success condition
> +  do_one_test (expected_success);
> +  return 0;
> +}
> +
> +/* This file references do_test above and contains the definition of
> +   the main function.  */
> +#include <support/test-driver.c>
> +
> --
> 2.21.0
>
>
> Girish Joshi
> girishjoshi.io
Carlos O'Donell - Nov. 30, 2019, 3:05 p.m.
On 11/30/19 9:47 AM, Girish Joshi wrote:
> Could anyone please review this patch?
Do you have copyright assignment with the FSF for your glibc work?

Please have a look through:
https://sourceware.org/glibc/wiki/Contribution%20checklist

If you have any questions I'm happy to help.

Patch

diff --git a/argp/Makefile b/argp/Makefile
index c97e4c307c..8206da8cd8 100644
--- a/argp/Makefile
+++ b/argp/Makefile
@@ -27,7 +27,7 @@  routines    = $(addprefix argp-, ba fmtstream
fs-xinl help parse pv \
                      pvh xinl eexst)

 tests        = argp-test tst-argp1 bug-argp1 tst-argp2 bug-argp2 \
-          tst-ldbl-argp
+          tst-ldbl-argp tst-argp3

 CFLAGS-argp-help.c += $(uses-callbacks) -fexceptions
 CFLAGS-argp-parse.c += $(uses-callbacks)
diff --git a/argp/argp-help.c b/argp/argp-help.c
index 85f5792bfe..68422127d2 100644
--- a/argp/argp-help.c
+++ b/argp/argp-help.c
@@ -1465,10 +1465,11 @@  argp_doc (const struct argp *argp, const
struct argp_state *state,
   size_t inp_text_limit = 0;
   const char *doc = dgettext (argp->argp_domain, argp->doc);
   const struct argp_child *child = argp->children;
+  char *vt = 0;

   if (doc)
     {
-      char *vt = strchr (doc, '\v');
+      vt = strchr (doc, '\v');
       inp_text = post ? (vt ? vt + 1 : 0) : doc;
       inp_text_limit = (!post && vt) ? (vt - doc) : 0;
     }
@@ -1499,7 +1500,12 @@  argp_doc (const struct argp *argp, const struct
argp_state *state,
       if (text == inp_text && inp_text_limit)
     __argp_fmtstream_write (stream, inp_text, inp_text_limit);
       else
-    __argp_fmtstream_puts (stream, text);
+      {
+        if ((!vt && !child) || (text == inp_text && !first_only))
+        {
+          __argp_fmtstream_puts (stream, text);
+        }
+      }

       if (__argp_fmtstream_point (stream) > __argp_fmtstream_lmargin (stream))
     __argp_fmtstream_putc (stream, '\n');
diff --git a/argp/tst-argp3.c b/argp/tst-argp3.c
new file mode 100644
index 0000000000..997072ba7d
--- /dev/null
+++ b/argp/tst-argp3.c
@@ -0,0 +1,68 @@ 
+/* Test for argparse with leading '\v' in the doc string.
+  Copyright (C) 2019 Free Software Foundation, Inc.
+
+  This program is free software: you can redistribute it and/or modify
+  it under the terms of the GNU General Public License as published by
+  the Free Software Foundation; either version 3 of the License, or
+  (at your option) any later version.
+
+  This program is distributed in the hope that it will be useful,
+  but WITHOUT ANY WARRANTY; without even the implied warranty of
+  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+  GNU General Public License for more details.
+
+  You should have received a copy of the GNU General Public License
+  along with this program.  If not, see <https://www.gnu.org/licenses/>.*/
+
+
+#include<stdlib.h>
+#include<argp.h>
+
+#include <support/capture_subprocess.h>
+#include <support/check.h>
+
+
+static char expected_success[] = "Usage: arp [OPTION...]\n\
+\n\
+  -?, --help                 Give this help list\n\
+      --usage                Give a short usage message\n\
+\n\
+this is post_doc\n\
+";
+char *argv[3] = { (char *) "arp", NULL, NULL };
+
+static void
+do_test_call (void)
+{
+  static char doc[] = "\vthis is post_doc";
+  static struct argp argp = {NULL, NULL, NULL, doc};
+
+  argp_parse (&argp, 2, argv, 0, 0, NULL);
+}
+
+static int
+do_one_test (const char *expected)
+{
+  struct support_capture_subprocess result;
+  result = support_capture_subprocess ((void *) &do_test_call, NULL);
+
+  TEST_COMPARE_STRING (result.out.buffer, expected);
+
+  return 0;
+}
+
+
+static int
+do_test (void)
+{
+  const char *argument = "--help";
+  argv[1] = (char *)argument;
+  // success condition
+  do_one_test (expected_success);
+  return 0;
+}
+
+/* This file references do_test above and contains the definition of
+   the main function.  */
+#include <support/test-driver.c>
+