Mailing List Archive: [PATCH 1/2] terminate config reading on 0x1A instead of just on pipe close

[PATCH 1/2] terminate config reading on 0x1A instead of just on pipe close

Jul 11, 2013, 3:18 PM

Post #1 of 4 (1536 views)

vpnc's config file processing logic uses EOF to determine when to stop
processing the config input, but if stdin is actually a pipe from a
controlling process, EOF only happens if the pipe is closed. Which
means the controlling process can't respond to any interactive requests
for information. So we need to add some other mechanism to indicate
that config processing is done that does not rely on closing stdin to
indicate this.

Also, getline() only returns on EOF (which has the problems described
above) or when it encounters sufficient newline characters;
unfortunately this precludes using getline() to handle single bytes.
Switch to fgetc() and build up the line ourselves so that we can
recognize a custom EOF character, 0x1A (Ctl-Z).
---
trunk/config.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 47 insertions(+), 7 deletions(-)

diff --git a/trunk/config.c b/trunk/config.c
index 7080630..e02a66f 100644
--- a/trunk/config.c
+++ b/trunk/config.c
@@ -96,6 +96,38 @@ void hex_dump(const char *str, const void *data, ssize_t len, const struct debug
printf("\n");
}

+/* mostly match getline() semantics but:
+ * 1) accept 0x1A as a line terminator
+ * 2) always allocate the buffer
+ * 3) max line size of 200 bytes
+ */
+static char *vpnc_getline(size_t *out_size, FILE *stream)
+{
+ char *buf;
+ size_t buflen = 200;
+ size_t llen = 0;
+ int c;
+
+ buf = malloc(buflen);
+ if (buf == NULL)
+ return NULL;
+
+ /* Read a line from the input */
+ while (llen < buflen - 1) {
+ c = fgetc (stream);
+ if (c == EOF || feof (stream)) {
+ free(buf);
+ return NULL;
+ }
+ buf[llen++] = (char) c;
+ if ((char) c == '\n' || (char) c == '\r' || (char) c == 0x1A)
+ break;
+ }
+ buf[llen] = '\0';
+ *out_size = llen;
+ return buf;
+}
+
static void config_deobfuscate(int obfuscated, int clear)
{
int ret, len = 0;
@@ -485,7 +517,6 @@ static void read_config_file(const char *name, const char **configs, int missing
{
FILE *f;
char *line = NULL;
- size_t line_length = 0;
int linenum = 0;
char *realname;

@@ -508,14 +539,22 @@ static void read_config_file(const char *name, const char **configs, int missing
error(1, errno, "couldn't open `%s'", realname);
}
for (;;) {
- ssize_t llen;
+ size_t llen = 0;
int i;

- llen = getline(&line, &line_length, f);
- if (llen == -1 && feof(f))
+ line = vpnc_getline(&llen, f);
+ if (line == NULL) {
+ if (feof (f))
+ break;
+
+ if (ferror (f))
+ error(1, errno, "reading `%s'", realname);
+ error(1, errno, "out of memory reading `%s'", realname);
+ }
+
+ /* 0x1A terminates reading configuration in addition to EOF */
+ if (llen == 1 && line[0] == 0x1A)
break;
- if (llen == -1)
- error(1, errno, "reading `%s'", realname);
if (llen > 0 && line[llen - 1] == '\n')
line[--llen] = 0;
if (llen > 0 && line[llen - 1] == '\r')
@@ -540,6 +579,7 @@ static void read_config_file(const char *name, const char **configs, int missing
if (config_names[i].name == NULL && line[0] != '#' && line[0] != 0)
error(0, 0, "warning: unknown configuration directive in %s at line %d",
realname, linenum);
+ free(line);
}
free(line);
free(realname);
@@ -804,7 +844,7 @@ void do_config(int argc, char **argv)
case CONFIG_IPSEC_GATEWAY:
case CONFIG_IPSEC_ID:
case CONFIG_XAUTH_USERNAME:
- getline(&s, &s_len, stdin);
+ s = vpnc_getline(&s_len, stdin);
}
if (s != NULL && strlen(s) > 0 && s[strlen(s) - 1] == '\n')
s[strlen(s) - 1] = 0;
--
1.8.1.4

_______________________________________________
vpnc-devel mailing list
vpnc-devel@unix-ag.uni-kl.de
https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel
http://www.unix-ag.uni-kl.de/~massar/vpnc/

Re: [PATCH 1/2] terminate config reading on 0x1A instead of just on pipe close [ In reply to ]

dcbw at redhat

Jul 16, 2013, 8:17 AM

Post #2 of 4 (1470 views)

Permalink

On Thu, 2013-07-11 at 17:18 -0500, Dan Williams wrote:
> vpnc's config file processing logic uses EOF to determine when to stop
> processing the config input, but if stdin is actually a pipe from a
> controlling process, EOF only happens if the pipe is closed. Which
> means the controlling process can't respond to any interactive requests
> for information. So we need to add some other mechanism to indicate
> that config processing is done that does not rely on closing stdin to
> indicate this.

Antonio, any thoughts on these patches? I'm happy to rework them as you
see fit. Thanks!

Dan

> Also, getline() only returns on EOF (which has the problems described
> above) or when it encounters sufficient newline characters;
> unfortunately this precludes using getline() to handle single bytes.
> Switch to fgetc() and build up the line ourselves so that we can
> recognize a custom EOF character, 0x1A (Ctl-Z).
> ---
> trunk/config.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 47 insertions(+), 7 deletions(-)
>
> diff --git a/trunk/config.c b/trunk/config.c
> index 7080630..e02a66f 100644
> --- a/trunk/config.c
> +++ b/trunk/config.c
> @@ -96,6 +96,38 @@ void hex_dump(const char *str, const void *data, ssize_t len, const struct debug
> printf("\n");
> }
>
> +/* mostly match getline() semantics but:
> + * 1) accept 0x1A as a line terminator
> + * 2) always allocate the buffer
> + * 3) max line size of 200 bytes
> + */
> +static char *vpnc_getline(size_t *out_size, FILE *stream)
> +{
> + char *buf;
> + size_t buflen = 200;
> + size_t llen = 0;
> + int c;
> +
> + buf = malloc(buflen);
> + if (buf == NULL)
> + return NULL;
> +
> + /* Read a line from the input */
> + while (llen < buflen - 1) {
> + c = fgetc (stream);
> + if (c == EOF || feof (stream)) {
> + free(buf);
> + return NULL;
> + }
> + buf[llen++] = (char) c;
> + if ((char) c == '\n' || (char) c == '\r' || (char) c == 0x1A)
> + break;
> + }
> + buf[llen] = '\0';
> + *out_size = llen;
> + return buf;
> +}
> +
> static void config_deobfuscate(int obfuscated, int clear)
> {
> int ret, len = 0;
> @@ -485,7 +517,6 @@ static void read_config_file(const char *name, const char **configs, int missing
> {
> FILE *f;
> char *line = NULL;
> - size_t line_length = 0;
> int linenum = 0;
> char *realname;
>
> @@ -508,14 +539,22 @@ static void read_config_file(const char *name, const char **configs, int missing
> error(1, errno, "couldn't open `%s'", realname);
> }
> for (;;) {
> - ssize_t llen;
> + size_t llen = 0;
> int i;
>
> - llen = getline(&line, &line_length, f);
> - if (llen == -1 && feof(f))
> + line = vpnc_getline(&llen, f);
> + if (line == NULL) {
> + if (feof (f))
> + break;
> +
> + if (ferror (f))
> + error(1, errno, "reading `%s'", realname);
> + error(1, errno, "out of memory reading `%s'", realname);
> + }
> +
> + /* 0x1A terminates reading configuration in addition to EOF */
> + if (llen == 1 && line[0] == 0x1A)
> break;
> - if (llen == -1)
> - error(1, errno, "reading `%s'", realname);
> if (llen > 0 && line[llen - 1] == '\n')
> line[--llen] = 0;
> if (llen > 0 && line[llen - 1] == '\r')
> @@ -540,6 +579,7 @@ static void read_config_file(const char *name, const char **configs, int missing
> if (config_names[i].name == NULL && line[0] != '#' && line[0] != 0)
> error(0, 0, "warning: unknown configuration directive in %s at line %d",
> realname, linenum);
> + free(line);
> }
> free(line);
> free(realname);
> @@ -804,7 +844,7 @@ void do_config(int argc, char **argv)
> case CONFIG_IPSEC_GATEWAY:
> case CONFIG_IPSEC_ID:
> case CONFIG_XAUTH_USERNAME:
> - getline(&s, &s_len, stdin);
> + s = vpnc_getline(&s_len, stdin);
> }
> if (s != NULL && strlen(s) > 0 && s[strlen(s) - 1] == '\n')
> s[strlen(s) - 1] = 0;

_______________________________________________
vpnc-devel mailing list
vpnc-devel@unix-ag.uni-kl.de
https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel
http://www.unix-ag.uni-kl.de/~massar/vpnc/

Re: [PATCH 1/2] terminate config reading on 0x1A instead of just on pipe close [ In reply to ]

dcbw at redhat

Jul 29, 2013, 9:42 PM

Post #3 of 4 (1462 views)

Permalink

On Tue, 2013-07-16 at 10:17 -0500, Dan Williams wrote:
> On Thu, 2013-07-11 at 17:18 -0500, Dan Williams wrote:
> > vpnc's config file processing logic uses EOF to determine when to stop
> > processing the config input, but if stdin is actually a pipe from a
> > controlling process, EOF only happens if the pipe is closed. Which
> > means the controlling process can't respond to any interactive requests
> > for information. So we need to add some other mechanism to indicate
> > that config processing is done that does not rely on closing stdin to
> > indicate this.
>
> Antonio, any thoughts on these patches? I'm happy to rework them as you
> see fit. Thanks!

One more ping; anything else you'd like me to fix up here?

Thanks!
Dan

> Dan
>
> > Also, getline() only returns on EOF (which has the problems described
> > above) or when it encounters sufficient newline characters;
> > unfortunately this precludes using getline() to handle single bytes.
> > Switch to fgetc() and build up the line ourselves so that we can
> > recognize a custom EOF character, 0x1A (Ctl-Z).
> > ---
> > trunk/config.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++-------
> > 1 file changed, 47 insertions(+), 7 deletions(-)
> >
> > diff --git a/trunk/config.c b/trunk/config.c
> > index 7080630..e02a66f 100644
> > --- a/trunk/config.c
> > +++ b/trunk/config.c
> > @@ -96,6 +96,38 @@ void hex_dump(const char *str, const void *data, ssize_t len, const struct debug
> > printf("\n");
> > }
> >
> > +/* mostly match getline() semantics but:
> > + * 1) accept 0x1A as a line terminator
> > + * 2) always allocate the buffer
> > + * 3) max line size of 200 bytes
> > + */
> > +static char *vpnc_getline(size_t *out_size, FILE *stream)
> > +{
> > + char *buf;
> > + size_t buflen = 200;
> > + size_t llen = 0;
> > + int c;
> > +
> > + buf = malloc(buflen);
> > + if (buf == NULL)
> > + return NULL;
> > +
> > + /* Read a line from the input */
> > + while (llen < buflen - 1) {
> > + c = fgetc (stream);
> > + if (c == EOF || feof (stream)) {
> > + free(buf);
> > + return NULL;
> > + }
> > + buf[llen++] = (char) c;
> > + if ((char) c == '\n' || (char) c == '\r' || (char) c == 0x1A)
> > + break;
> > + }
> > + buf[llen] = '\0';
> > + *out_size = llen;
> > + return buf;
> > +}
> > +
> > static void config_deobfuscate(int obfuscated, int clear)
> > {
> > int ret, len = 0;
> > @@ -485,7 +517,6 @@ static void read_config_file(const char *name, const char **configs, int missing
> > {
> > FILE *f;
> > char *line = NULL;
> > - size_t line_length = 0;
> > int linenum = 0;
> > char *realname;
> >
> > @@ -508,14 +539,22 @@ static void read_config_file(const char *name, const char **configs, int missing
> > error(1, errno, "couldn't open `%s'", realname);
> > }
> > for (;;) {
> > - ssize_t llen;
> > + size_t llen = 0;
> > int i;
> >
> > - llen = getline(&line, &line_length, f);
> > - if (llen == -1 && feof(f))
> > + line = vpnc_getline(&llen, f);
> > + if (line == NULL) {
> > + if (feof (f))
> > + break;
> > +
> > + if (ferror (f))
> > + error(1, errno, "reading `%s'", realname);
> > + error(1, errno, "out of memory reading `%s'", realname);
> > + }
> > +
> > + /* 0x1A terminates reading configuration in addition to EOF */
> > + if (llen == 1 && line[0] == 0x1A)
> > break;
> > - if (llen == -1)
> > - error(1, errno, "reading `%s'", realname);
> > if (llen > 0 && line[llen - 1] == '\n')
> > line[--llen] = 0;
> > if (llen > 0 && line[llen - 1] == '\r')
> > @@ -540,6 +579,7 @@ static void read_config_file(const char *name, const char **configs, int missing
> > if (config_names[i].name == NULL && line[0] != '#' && line[0] != 0)
> > error(0, 0, "warning: unknown configuration directive in %s at line %d",
> > realname, linenum);
> > + free(line);
> > }
> > free(line);
> > free(realname);
> > @@ -804,7 +844,7 @@ void do_config(int argc, char **argv)
> > case CONFIG_IPSEC_GATEWAY:
> > case CONFIG_IPSEC_ID:
> > case CONFIG_XAUTH_USERNAME:
> > - getline(&s, &s_len, stdin);
> > + s = vpnc_getline(&s_len, stdin);
> > }
> > if (s != NULL && strlen(s) > 0 && s[strlen(s) - 1] == '\n')
> > s[strlen(s) - 1] = 0;
>
>
> _______________________________________________
> vpnc-devel mailing list
> vpnc-devel@unix-ag.uni-kl.de
> https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel
> http://www.unix-ag.uni-kl.de/~massar/vpnc/

_______________________________________________
vpnc-devel mailing list
vpnc-devel@unix-ag.uni-kl.de
https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel
http://www.unix-ag.uni-kl.de/~massar/vpnc/

Re: [PATCH 1/2] terminate config reading on 0x1A instead of just on pipe close [ In reply to ]

borneo.antonio at gmail

Nov 30, 2013, 3:13 AM

Post #4 of 4 (1338 views)

Permalink

Hi Dan,

at last I'm back on this patch.

On Fri, Jul 12, 2013 at 6:18 AM, Dan Williams <dcbw@redhat.com> wrote:
> vpnc's config file processing logic uses EOF to determine when to stop
> processing the config input, but if stdin is actually a pipe from a
> controlling process, EOF only happens if the pipe is closed. Which
> means the controlling process can't respond to any interactive requests
> for information. So we need to add some other mechanism to indicate
> that config processing is done that does not rely on closing stdin to
> indicate this.
>
> Also, getline() only returns on EOF (which has the problems described
> above) or when it encounters sufficient newline characters;
> unfortunately this precludes using getline() to handle single bytes.
> Switch to fgetc() and build up the line ourselves so that we can
> recognize a custom EOF character, 0x1A (Ctl-Z).

Ctrl-Z is a DOS/Windows convension.
Ctrl-D as first char of a line is more suitable in Unix world
http://en.wikipedia.org/wiki/End-of-file
I do not have specific preference, but comments are welcome

Would be good to have a note about this new behavior in man-page.

Instead of using magic number, consider including
sys/ttydefaults.h
and use the macros CEOF for Ctrl-D or CSUPS for Ctrl-Z.

> ---
> trunk/config.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 47 insertions(+), 7 deletions(-)
>
> diff --git a/trunk/config.c b/trunk/config.c
> index 7080630..e02a66f 100644
> --- a/trunk/config.c
> +++ b/trunk/config.c
> @@ -96,6 +96,38 @@ void hex_dump(const char *str, const void *data, ssize_t len, const struct debug
> printf("\n");
> }
>
> +/* mostly match getline() semantics but:
> + * 1) accept 0x1A as a line terminator
> + * 2) always allocate the buffer
> + * 3) max line size of 200 bytes
> + */
> +static char *vpnc_getline(size_t *out_size, FILE *stream)
> +{
> + char *buf;
> + size_t buflen = 200;

Better using a macro in place of magic number, e.g.
#define GETLINE_MAX_BUFLEN
and
size_t buflen = GETLINE_MAX_BUFLEN;

> + size_t llen = 0;
> + int c;
> +
> + buf = malloc(buflen);
> + if (buf == NULL)
> + return NULL;
> +
> + /* Read a line from the input */
> + while (llen < buflen - 1) {
> + c = fgetc (stream);
> + if (c == EOF || feof (stream)) {
> + free(buf);
> + return NULL;
> + }

A line that doesn't end with '\n' but with EOF would be discarded.
The original getline(), instead, will return the line without trailing
'\n'. Only following call to getline() will return -1 due to EOF.
The new code does not mimic getline() anymore. Is this intentional?

Thanks,
Antonio

> + buf[llen++] = (char) c;
> + if ((char) c == '\n' || (char) c == '\r' || (char) c == 0x1A)
> + break;
> + }
> + buf[llen] = '\0';
> + *out_size = llen;
> + return buf;
> +}
> +
> static void config_deobfuscate(int obfuscated, int clear)
> {
> int ret, len = 0;
> @@ -485,7 +517,6 @@ static void read_config_file(const char *name, const char **configs, int missing
> {
> FILE *f;
> char *line = NULL;
> - size_t line_length = 0;
> int linenum = 0;
> char *realname;
>
> @@ -508,14 +539,22 @@ static void read_config_file(const char *name, const char **configs, int missing
> error(1, errno, "couldn't open `%s'", realname);
> }
> for (;;) {
> - ssize_t llen;
> + size_t llen = 0;
> int i;
>
> - llen = getline(&line, &line_length, f);
> - if (llen == -1 && feof(f))
> + line = vpnc_getline(&llen, f);
> + if (line == NULL) {
> + if (feof (f))
> + break;
> +
> + if (ferror (f))
> + error(1, errno, "reading `%s'", realname);
> + error(1, errno, "out of memory reading `%s'", realname);
> + }
> +
> + /* 0x1A terminates reading configuration in addition to EOF */
> + if (llen == 1 && line[0] == 0x1A)
> break;
> - if (llen == -1)
> - error(1, errno, "reading `%s'", realname);
> if (llen > 0 && line[llen - 1] == '\n')
> line[--llen] = 0;
> if (llen > 0 && line[llen - 1] == '\r')
> @@ -540,6 +579,7 @@ static void read_config_file(const char *name, const char **configs, int missing
> if (config_names[i].name == NULL && line[0] != '#' && line[0] != 0)
> error(0, 0, "warning: unknown configuration directive in %s at line %d",
> realname, linenum);
> + free(line);
> }
> free(line);
> free(realname);
> @@ -804,7 +844,7 @@ void do_config(int argc, char **argv)
> case CONFIG_IPSEC_GATEWAY:
> case CONFIG_IPSEC_ID:
> case CONFIG_XAUTH_USERNAME:
> - getline(&s, &s_len, stdin);
> + s = vpnc_getline(&s_len, stdin);
> }
> if (s != NULL && strlen(s) > 0 && s[strlen(s) - 1] == '\n')
> s[strlen(s) - 1] = 0;
> --
> 1.8.1.4
>
>
> _______________________________________________
> vpnc-devel mailing list
> vpnc-devel@unix-ag.uni-kl.de
> https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel
> http://www.unix-ag.uni-kl.de/~massar/vpnc/
_______________________________________________
vpnc-devel mailing list
vpnc-devel@unix-ag.uni-kl.de
https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel
http://www.unix-ag.uni-kl.de/~massar/vpnc/