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

Message ID CALkY8p960_gyY_Q1C+FPKuRYYyBCyp7V=PZpu8XaNW5Mxkxdgg@mail.gmail.com
State New
Headers show
Series
  • argp: argp.doc prints incorrectly when it starts with "\v" [BZ #19038]
Related show

Commit Message

Girish Joshi May 6, 2019, 8:55 a.m. UTC
In argp-help.c `char *vt` is being initialized only once. 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 `first_only` to the function `argp_doc` is false and the
complete doc needs to be printed.

     }
@@ -1498,8 +1499,11 @@ 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);
+      else{
+        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');

Comments

Florian Weimer May 6, 2019, 12:37 p.m. UTC | #1
* Girish Joshi:

> In argp-help.c `char *vt` is being initialized only once. 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 `first_only` to the function `argp_doc` is false and the
> complete doc needs to be printed.

I assume this bug user-visible, so it should have a bug in Bugzilla
<https://sourceware.org/bugzilla/>.  Would you please file one?

Could you write a test case for this?  Or at least post how to reproduce
the issue in a minimal program?

Thanks,
Florian
Girish Joshi May 6, 2019, 1:38 p.m. UTC | #2
Hi Florian,

> I assume this bug user-visible, so it should have a bug in Bugzilla
> <https://sourceware.org/bugzilla/>.  Would you please file one?
>
Here[1] is a Bugzilla entry for this.

> Could you write a test case for this?  Or at least post how to reproduce
> the issue in a minimal program?
>
It can be reproduced with following code.

#include<stdlib.h>
#include<argp.h>

static char doc[] = "\vthis is post_doc";
static struct argp argp = {NULL, NULL, NULL, doc};

int main(int argc, char *args[]){
     argp_parse(&argp, argc, args, 0, 0, NULL);
}

the output of the above code looks like:

this is post_doc

  -?, --help                 Give this help list
      --usage                Give a short usage message

this is post_doc

In this case the doc is being printed two times.
As mentioned in the comment by Kenny Stauffer on Bugzilla the first
occurrence of `this is post_doc` is erroneous.

If we change the doc[] in the above code to something else like

static char doc[] = "this is pre_doc\vthis is post_doc";

We get the output like:

this is pre_doc

  -?, --help                 Give this help list
      --usage                Give a short usage message

this is post_doc

[1]: https://sourceware.org/bugzilla/show_bug.cgi?id=19038
Girish Joshi May 30, 2019, 8:14 a.m. UTC | #3
Could someone please review this patch?
Thanks.
Adhemerval Zanella May 30, 2019, 12:52 p.m. UTC | #4
On 30/05/2019 05:14, Girish Joshi wrote:
> Could someone please review this patch?
> Thanks.
> 

My understanding is Florian has asked if you could write a testcase
using glibc libsupport and resend the patch along with the testcase.
It will also need a ChangeLog entry.
Girish Joshi July 5, 2019, 9:01 a.m. UTC | #5
Hello,
Following is the patch with ChangeLog entry and a test case.
Please let me know if any changes are needed in it.

diff --git a/ChangeLog b/ChangeLog
index fbe5cd8648..7b11db3127 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2019-07-04  Girish Joshi  <girish946@gmail.com>
+
+    [BZ #19038]
+    * argp/argp-help.c: Added validation for printing pre and post doc.
+    * argp/tst-argp-help.c: Added testcase for docstring help/usage.
+
 2019-07-04  Andreas Schwab  <schwab@suse.de>

     [BZ #24484]
diff --git a/argp/Makefile b/argp/Makefile
index d5c2d77658..073ea3049e 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-argp-help

 CFLAGS-argp-help.c += $(uses-callbacks) -fexceptions
 CFLAGS-argp-parse.c += $(uses-callbacks)
@@ -35,5 +35,6 @@ CFLAGS-argp-fmtstream.c += -fexceptions

 bug-argp1-ARGS = -- --help
 bug-argp2-ARGS = -- -d 111 --dstaddr 222 -p 333 --peer 444
+tst-argp-help-ARGS = -- --help

 include ../Rules
diff --git a/argp/argp-help.c b/argp/argp-help.c
index 3b1727c4aa..ee4d247824 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;
     }
@@ -1498,8 +1499,11 @@ 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);
+      else{
+        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-argp-help.c b/argp/tst-argp-help.c
new file mode 100644
index 0000000000..7abb97f8df
--- /dev/null
+++ b/argp/tst-argp-help.c
@@ -0,0 +1,39 @@
+/* Copyright (C) 2002-2019 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+   Contributed by Ulrich Drepper <drepper@redhat.com>, 2002.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library 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
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <argp.h>
+
+static int
+do_test (void)
+{
+
+  int argc = 2;
+  char *argv[3] = { (char *) "tst-argp-help", (char *) "--help", NULL };
+  int remaining;
+
+  static char doc[] = "\vthis is post_doc";
+  static struct argp argp = {NULL, NULL, NULL, doc};
+
+  /* Parse and process arguments.  */
+  argp_parse(&argp, argc, argv, 0, &remaining, NULL);
+
+  return 0;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"

Thanks.
Girish Joshi July 5, 2019, 10:34 a.m. UTC | #6
My bad, I had copied the license part from another file and did not
change the Email-Id and author name.
> Following is the patch with ChangeLog entry and a test case.
> Please let me know if any changes are needed in it.

diff --git a/ChangeLog b/ChangeLog
index fbe5cd8648..7b11db3127 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2019-07-04  Girish Joshi  <girish946@gmail.com>
+
+    [BZ #19038]
+    * argp/argp-help.c: Added validation for printing pre and post doc.
+    * argp/tst-argp-help.c: Added testcase for docstring help/usage.
+
 2019-07-04  Andreas Schwab  <schwab@suse.de>

     [BZ #24484]
diff --git a/argp/Makefile b/argp/Makefile
index d5c2d77658..073ea3049e 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-argp-help

 CFLAGS-argp-help.c += $(uses-callbacks) -fexceptions
 CFLAGS-argp-parse.c += $(uses-callbacks)
@@ -35,5 +35,6 @@ CFLAGS-argp-fmtstream.c += -fexceptions

 bug-argp1-ARGS = -- --help
 bug-argp2-ARGS = -- -d 111 --dstaddr 222 -p 333 --peer 444
+tst-argp-help-ARGS = -- --help

 include ../Rules
diff --git a/argp/argp-help.c b/argp/argp-help.c
index 3b1727c4aa..ee4d247824 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;
     }
@@ -1498,8 +1499,11 @@ 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);
+      else{
+        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-argp-help.c b/argp/tst-argp-help.c
new file mode 100644
index 0000000000..3bfeb0219c
--- /dev/null
+++ b/argp/tst-argp-help.c
@@ -0,0 +1,39 @@
+/* Copyright (C) 2002-2019 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+   Contributed by Girish Joshi <girish946@gmail.com>, 2019.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library 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
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <argp.h>
+
+static int
+do_test (void)
+{
+
+  int argc = 2;
+  char *argv[3] = { (char *) "tst-argp-help", (char *) "--help", NULL };
+  int remaining;
+
+  static char doc[] = "\vthis is post_doc";
+  static struct argp argp = {NULL, NULL, NULL, doc};
+
+  /* Parse and process arguments.  */
+  argp_parse(&argp, argc, argv, 0, &remaining, NULL);
+
+  return 0;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"

Patch
diff mbox series

diff --git a/argp/argp-help.c b/argp/argp-help.c
index 3b1727c4aa..ee4d247824 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;