[Openvpn-devel] Fix line number reporting on config file errors after <inline> segments

Message ID 20200920090929.8321-1-gert@greenie.muc.de
State New
Headers show
Series
  • [Openvpn-devel] Fix line number reporting on config file errors after <inline> segments
Related show

Commit Message

Gert Doering Sept. 20, 2020, 9:09 a.m.
<inline> segments neglected to increment the "current line number
in config file" variable (line_num), so after the first <inline>,
errors reported have the wrong line number.

Fix by introducing an extra argument to the check_inline*() /
read_inline_file() call chain: "so many lines in the inline block".

Trac: #1325

Signed-off-by: Gert Doering <gert@greenie.muc.de>
---
 src/openvpn/options.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

Comments

Antonio Quartulli Sept. 21, 2020, 7:22 a.m. | #1
Hi,

On 20/09/2020 11:09, Gert Doering wrote:
> <inline> segments neglected to increment the "current line number
> in config file" variable (line_num), so after the first <inline>,
> errors reported have the wrong line number.
> 
> Fix by introducing an extra argument to the check_inline*() /
> read_inline_file() call chain: "so many lines in the inline block".
> 
> Trac: #1325
> 
> Signed-off-by: Gert Doering <gert@greenie.muc.de>
> ---
>  src/openvpn/options.c | 29 ++++++++++++++++++++---------
>  1 file changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index f370a2bb..af517bab 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -4634,7 +4634,8 @@ in_src_get(const struct in_src *is, char *line, const int size)
>  }
>  
>  static char *
> -read_inline_file(struct in_src *is, const char *close_tag, struct gc_arena *gc)
> +read_inline_file(struct in_src *is, const char *close_tag,
> +                 int *num_lines, struct gc_arena *gc)
>  {
>      char line[OPTION_LINE_SIZE];
>      struct buffer buf = alloc_buf(8*OPTION_LINE_SIZE);
> @@ -4643,6 +4644,7 @@ read_inline_file(struct in_src *is, const char *close_tag, struct gc_arena *gc)
>  
>      while (in_src_get(is, line, sizeof(line)))
>      {
> +        (*num_lines)++;
>          char *line_ptr = line;
>          /* Remove leading spaces */
>          while (isspace(*line_ptr))
> @@ -4677,9 +4679,11 @@ read_inline_file(struct in_src *is, const char *close_tag, struct gc_arena *gc)
>  }
>  
>  static bool
> -check_inline_file(struct in_src *is, char *p[], struct gc_arena *gc)
> +check_inline_file(struct in_src *is, char *p[],
> +                  int *num_lines, struct gc_arena *gc)

Sorry for not chiming in earlier, but honestly I believe your other
option would be "cleaner". The other option being "return int instead of
bool, where the returned value is the number of lines of the inline'd
material.

If 0, we know there was no inline'd material, thus we can treat that
result as "not inline'd".

Anyway, this is just my opinion. If you believe this is a better
approach, I am fine with it too.

>  {
>      bool is_inline = false;
> +    *num_lines = 0;
>  
>      if (p[0] && !p[1])
>      {
> @@ -4692,7 +4696,7 @@ check_inline_file(struct in_src *is, char *p[], struct gc_arena *gc)
>              p[0] = string_alloc(arg + 1, gc);
>              close_tag = alloc_buf(strlen(p[0]) + 4);
>              buf_printf(&close_tag, "</%s>", p[0]);
> -            p[1] = read_inline_file(is, BSTR(&close_tag), gc);
> +            p[1] = read_inline_file(is, BSTR(&close_tag), num_lines, gc);

am I wrong or here we should add 1 to num_lines to include the opening
tag that was parsed *before* entering read_inline_file()?

Regards,
Gert Doering Sept. 21, 2020, 7:50 a.m. | #2
Hi,

On Mon, Sep 21, 2020 at 09:22:38AM +0200, Antonio Quartulli wrote:
> Sorry for not chiming in earlier, but honestly I believe your other
> option would be "cleaner". The other option being "return int instead of
> bool, where the returned value is the number of lines of the inline'd
> material.
>
> If 0, we know there was no inline'd material, thus we can treat that
> result as "not inline'd".

We could do that, indeed.  I was torn between both variants - this one
is a bit more intrusive (because it adds a new argument to so many
functions), but on the other hand, the bool stays a bool and is not
magically overloaded - which would make the call to add_option() 
either "even longer", or if you rely on magic "we just pass the integer
to a bool-expecting function" much harder to understand.

> Anyway, this is just my opinion. If you believe this is a better
> approach, I am fine with it too.

I wasn't sure.  In the end, this one didn't feel so bad, so this is
what we have.

Maybe I'll do the other one as well, for fun, and to compare :-)

> > @@ -4692,7 +4696,7 @@ check_inline_file(struct in_src *is, char *p[], struct gc_arena *gc)
> >              p[0] = string_alloc(arg + 1, gc);
> >              close_tag = alloc_buf(strlen(p[0]) + 4);
> >              buf_printf(&close_tag, "</%s>", p[0]);
> > -            p[1] = read_inline_file(is, BSTR(&close_tag), gc);
> > +            p[1] = read_inline_file(is, BSTR(&close_tag), num_lines, gc);
> 
> am I wrong or here we should add 1 to num_lines to include the opening
> tag that was parsed *before* entering read_inline_file()?

The opening tag is parsed as "regular" line in read_config_file(), and as 
such, the "++line_num" in there applies.

(I have tested this :-) - nothing more embarrassing than a patch to fix
line numbers actually still printing wrong line numbers)

gert
Antonio Quartulli Sept. 21, 2020, 1:45 p.m. | #3
Hi,

On 21/09/2020 09:50, Gert Doering wrote:
> Hi,
> 
> On Mon, Sep 21, 2020 at 09:22:38AM +0200, Antonio Quartulli wrote:
>> Sorry for not chiming in earlier, but honestly I believe your other
>> option would be "cleaner". The other option being "return int instead of
>> bool, where the returned value is the number of lines of the inline'd
>> material.
>>
>> If 0, we know there was no inline'd material, thus we can treat that
>> result as "not inline'd".
> 
> We could do that, indeed.  I was torn between both variants - this one
> is a bit more intrusive (because it adds a new argument to so many
> functions), but on the other hand, the bool stays a bool and is not
> magically overloaded - which would make the call to add_option() 
> either "even longer", or if you rely on magic "we just pass the integer
> to a bool-expecting function" much harder to understand.
> 
>> Anyway, this is just my opinion. If you believe this is a better
>> approach, I am fine with it too.
> 
> I wasn't sure.  In the end, this one didn't feel so bad, so this is
> what we have.
> 
> Maybe I'll do the other one as well, for fun, and to compare :-)
> 
>>> @@ -4692,7 +4696,7 @@ check_inline_file(struct in_src *is, char *p[], struct gc_arena *gc)
>>>              p[0] = string_alloc(arg + 1, gc);
>>>              close_tag = alloc_buf(strlen(p[0]) + 4);
>>>              buf_printf(&close_tag, "</%s>", p[0]);
>>> -            p[1] = read_inline_file(is, BSTR(&close_tag), gc);
>>> +            p[1] = read_inline_file(is, BSTR(&close_tag), num_lines, gc);
>>
>> am I wrong or here we should add 1 to num_lines to include the opening
>> tag that was parsed *before* entering read_inline_file()?
> 
> The opening tag is parsed as "regular" line in read_config_file(), and as 
> such, the "++line_num" in there applies.
> 
> (I have tested this :-) - nothing more embarrassing than a patch to fix
> line numbers actually still printing wrong line numbers)
> 

Ok, then for me this patch is good to go.

FTR on IRC we discussed the possibility of using the "other approach".
Gert may give it a try and decide if he wants to send that version to
the ml as well.

In any case, this patch gets my blessing:

Acked-by: Antonio Quartulli <a@unstable.cc>

Patch

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index f370a2bb..af517bab 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -4634,7 +4634,8 @@  in_src_get(const struct in_src *is, char *line, const int size)
 }
 
 static char *
-read_inline_file(struct in_src *is, const char *close_tag, struct gc_arena *gc)
+read_inline_file(struct in_src *is, const char *close_tag,
+                 int *num_lines, struct gc_arena *gc)
 {
     char line[OPTION_LINE_SIZE];
     struct buffer buf = alloc_buf(8*OPTION_LINE_SIZE);
@@ -4643,6 +4644,7 @@  read_inline_file(struct in_src *is, const char *close_tag, struct gc_arena *gc)
 
     while (in_src_get(is, line, sizeof(line)))
     {
+        (*num_lines)++;
         char *line_ptr = line;
         /* Remove leading spaces */
         while (isspace(*line_ptr))
@@ -4677,9 +4679,11 @@  read_inline_file(struct in_src *is, const char *close_tag, struct gc_arena *gc)
 }
 
 static bool
-check_inline_file(struct in_src *is, char *p[], struct gc_arena *gc)
+check_inline_file(struct in_src *is, char *p[],
+                  int *num_lines, struct gc_arena *gc)
 {
     bool is_inline = false;
+    *num_lines = 0;
 
     if (p[0] && !p[1])
     {
@@ -4692,7 +4696,7 @@  check_inline_file(struct in_src *is, char *p[], struct gc_arena *gc)
             p[0] = string_alloc(arg + 1, gc);
             close_tag = alloc_buf(strlen(p[0]) + 4);
             buf_printf(&close_tag, "</%s>", p[0]);
-            p[1] = read_inline_file(is, BSTR(&close_tag), gc);
+            p[1] = read_inline_file(is, BSTR(&close_tag), num_lines, gc);
             p[2] = NULL;
             free_buf(&close_tag);
             is_inline = true;
@@ -4702,22 +4706,23 @@  check_inline_file(struct in_src *is, char *p[], struct gc_arena *gc)
 }
 
 static bool
-check_inline_file_via_fp(FILE *fp, char *p[], struct gc_arena *gc)
+check_inline_file_via_fp(FILE *fp, char *p[],
+                         int *num_lines, struct gc_arena *gc)
 {
     struct in_src is;
     is.type = IS_TYPE_FP;
     is.u.fp = fp;
-    return check_inline_file(&is, p, gc);
+    return check_inline_file(&is, p, num_lines, gc);
 }
 
 static bool
 check_inline_file_via_buf(struct buffer *multiline, char *p[],
-                          struct gc_arena *gc)
+                          int *num_lines, struct gc_arena *gc)
 {
     struct in_src is;
     is.type = IS_TYPE_BUF;
     is.u.multiline = multiline;
-    return check_inline_file(&is, p, gc);
+    return check_inline_file(&is, p, num_lines, gc);
 }
 
 static void
@@ -4782,12 +4787,15 @@  read_config_file(struct options *options,
                 if (parse_line(line + offset, p, SIZE(p)-1, file, line_num, msglevel, &options->gc))
                 {
                     bool is_inline;
+                    int lines;
 
                     bypass_doubledash(&p[0]);
-                    is_inline = check_inline_file_via_fp(fp, p, &options->gc);
+                    is_inline = check_inline_file_via_fp(fp, p,
+                                                         &lines, &options->gc);
                     add_option(options, p, is_inline, file, line_num, level,
                                msglevel, permission_mask, option_types_found,
                                es);
+                    line_num += lines;
                 }
             }
             if (fp != stdin)
@@ -4831,11 +4839,14 @@  read_config_string(const char *prefix,
         if (parse_line(line, p, SIZE(p)-1, prefix, line_num, msglevel, &options->gc))
         {
             bool is_inline;
+            int lines;
 
             bypass_doubledash(&p[0]);
-            is_inline = check_inline_file_via_buf(&multiline, p, &options->gc);
+            is_inline = check_inline_file_via_buf(&multiline, p,
+                                                  &lines, &options->gc);
             add_option(options, p, is_inline, prefix, line_num, 0, msglevel,
                        permission_mask, option_types_found, es);
+            line_num += lines;
         }
         CLEAR(p);
     }