[PATCH v2] [RFC] libdep plugin: fix bugs in parser and drop escaping
Harmen Stoppels
me@harmenstoppels.nl
Thu Jun 20 07:49:19 GMT 2024
Second version of this patch addresses the following:
1. fix another buffer overflow in libdep_plugin.c when the libdep argument
string was tab separated instead of ` ` white space.
2. drops handling of `\`: previously the parser simply dropped that character,
it did not escape anything, and it was impossible to specify `-Lfoo\bar`
with a literal `\` in the path. Further `\` triggered a bug where it did not
memmove the trailing null byte. So it is not breaking to make the parser
treat `\` like a literal character. There is no need for an escape character
because single and double quotes allow one to express any character as an
argument, include white space and quotes.
3. unterminated quotes are now a warning, the plugin does nothing in that case.
4. make the parser linear time. memmove made it quadratic worst case.
PR ld/31906
* libdep_plugin.c (str2vec): Fix bug where null byte was not copied on
memmove during quote handling and escaping, causing repeat of the last
character in the last argument. Fix buffer overflow in **res when
arguments were separated by `\t` instead of ` `. Remove handling of
the escape character `\`, as it made it impossible to specify paths
containing `\` -- the implementation merely dropped `\`, and was
affected by the memmove bug, so this should not be breaking; just
single and double quotes are sufficient to deal with white space and
quote characters, there is no need for escaping. Handle syntax errors
on unterminated quotes. Make the parser linear time instead of
quadratic.
Signed-off-by: Harmen Stoppels <me@harmenstoppels.nl>
---
ld/libdep_plugin.c | 153 ++++++++++++++++++---------------------------
1 file changed, 61 insertions(+), 92 deletions(-)
diff --git a/ld/libdep_plugin.c b/ld/libdep_plugin.c
index 414f3cffe12..65372abdcb1 100644
--- a/ld/libdep_plugin.c
+++ b/ld/libdep_plugin.c
@@ -132,83 +132,56 @@ get_libdeps (int fd)
return rc;
}
-/* Turn a string into an argvec. */
-static char **
-str2vec (char *in)
+/* Parse arguments in-place as contiguous C-strings
+ and return the number of arguments */
+int
+parse_libdep (char *str)
{
- char **res;
- char *s, *first, *end;
- char *sq, *dq;
- int i;
-
- end = in + strlen (in);
- s = in;
- while (isspace ((unsigned char) *s)) s++;
- first = s;
-
- i = 1;
- while ((s = strchr (s, ' ')))
+ char *src, *dst;
+ char quote;
+ int narg;
+ src = dst = str;
+ for (; isspace ((unsigned char) *src); ++src);
+ if (*src == '\0')
+ return 0;
+ narg = 1;
+ quote = 0;
+ while (*src)
{
- s++;
- i++;
- }
- res = (char **)malloc ((i+1) * sizeof (char *));
- if (!res)
- return res;
-
- i = 0;
- sq = NULL;
- dq = NULL;
- res[0] = first;
- for (s = first; *s; s++)
- {
- if (*s == '\\')
- {
- memmove (s, s+1, end-s-1);
- end--;
- }
- if (isspace ((unsigned char) *s))
- {
- if (sq || dq)
- continue;
- *s++ = '\0';
- while (isspace ((unsigned char) *s)) s++;
- if (*s)
- res[++i] = s;
- }
- if (*s == '\'' && !dq)
+ if (*src == '\'' || *src == '\"')
{
- if (sq)
+ if (!quote)
+ quote = *src++;
+
+ else if (*src == quote)
{
- memmove (sq, sq+1, s-sq-1);
- memmove (s-2, s+1, end-s-1);
- end -= 2;
- s--;
- sq = NULL;
+ ++src;
+ quote = 0;
}
+
else
- {
- sq = s;
- }
+ *dst++ = *src++;
}
- if (*s == '"' && !sq)
+
+ else if (!quote && isspace ((unsigned char) *src))
{
- if (dq)
- {
- memmove (dq, dq+1, s-dq-1);
- memmove (s-2, s+1, end-s-1);
- end -= 2;
- s--;
- dq = NULL;
- }
- else
- {
- dq = s;
- }
+ ++narg;
+ ++src;
+ *dst++ = '\0';
+ for (; isspace ((unsigned char) *src); ++src);
}
+
+ else
+ *dst++ = *src++;
}
- res[++i] = NULL;
- return res;
+ *dst = '\0';
+ if (quote)
+ {
+ TV_MESSAGE (LDPL_WARNING,
+ "libdep syntax error: unterminated quoted string");
+ return 0;
+ }
+ return narg;
}
static char *prevfile;
@@ -260,39 +233,35 @@ static enum ld_plugin_status
onall_symbols_read (void)
{
linerec *lr;
- char **vec;
+ int nargs;
+ char const *arg;
enum ld_plugin_status rv = LDPS_OK;
while ((lr = line_head))
{
line_head = lr->next;
- vec = str2vec (lr->line);
- if (vec)
+ nargs = parse_libdep (lr->line);
+ arg = lr->line;
+ int i;
+ for (i = 0; i < nargs; i++, arg = strchr (arg, '\0') + 1)
{
- int i;
- for (i = 0; vec[i]; i++)
+ if (arg[0] != '-')
+ {
+ TV_MESSAGE (LDPL_WARNING, "ignoring libdep argument %s", arg);
+ fflush (NULL);
+ continue;
+ }
+ if (arg[1] == 'l')
+ rv = tv_add_input_library (arg + 2);
+ else if (arg[1] == 'L')
+ rv = tv_set_extra_library_path (arg + 2);
+ else
{
- if (vec[i][0] != '-')
- {
- TV_MESSAGE (LDPL_WARNING, "ignoring libdep argument %s",
- vec[i]);
- fflush (NULL);
- continue;
- }
- if (vec[i][1] == 'l')
- rv = tv_add_input_library (vec[i]+2);
- else if (vec[i][1] == 'L')
- rv = tv_set_extra_library_path (vec[i]+2);
- else
- {
- TV_MESSAGE (LDPL_WARNING, "ignoring libdep argument %s",
- vec[i]);
- fflush (NULL);
- }
- if (rv != LDPS_OK)
- break;
+ TV_MESSAGE (LDPL_WARNING, "ignoring libdep argument %s", arg);
+ fflush (NULL);
}
- free (vec);
+ if (rv != LDPS_OK)
+ break;
}
free (lr);
}
--
2.40.1
On Wednesday, June 19th, 2024 at 5:07 PM, Harmen Stoppels <me@harmenstoppels.nl> wrote:
>
>
> There's another buffer overflow bug if the input is tab-separated.
>
> The char **res array is allocated based on number of literal spaces,
> but should use `isspace`.
>
> I have another fix that works allocation free and in linear time in the
> input string, it just counts the number of arguments and packs them as
> contiguous C-strings. On the call site it advances with
> `arg = strchr(arg, '\\0') + 1`.
>
> I would also like feedback on character escaping. The current implementation
> is different from how a shell behaves. How should it behave?
>
>
> On Wednesday, June 19th, 2024 at 12:03 PM, Harmen Stoppels me@harmenstoppels.nl wrote:
>
> > Fix bugs in the __.LIBDEP string to argument list parser in the libdep
> > plugin.
> >
> > PR ld/31906
> > * libdep_plugin.c (st2vec): fix bug where null byte was not copied
> > on memmove, repeating the last character. Allow literal \ by escaping
> > it with \\. Allow escaping of quotes inside quoted strings. Fix out
> > of bounds errors leading to segfaults on input strings like
> > `-L/'a\\\\'b'`.
> >
> > Signed-off-by: Harmen Stoppels me@harmenstoppels.nl
> >
> > ---
> > ld/libdep_plugin.c | 63 +++++++++++++++++++---------------------------
> > 1 file changed, 26 insertions(+), 37 deletions(-)
> >
> > diff --git a/ld/libdep_plugin.c b/ld/libdep_plugin.c
> > index 414f3cffe12..44fb7cbd588 100644
> > --- a/ld/libdep_plugin.c
> > +++ b/ld/libdep_plugin.c
> > @@ -138,7 +138,7 @@ str2vec (char *in)
> > {
> > char **res;
> > char *s, *first, *end;
> > - char *sq, *dq;
> > + char *quote;
> > int i;
> >
> > end = in + strlen (in);
> > @@ -157,55 +157,44 @@ str2vec (char *in)
> > return res;
> >
> > i = 0;
> > - sq = NULL;
> > - dq = NULL;
> > + quote = NULL;
> > res[0] = first;
> > - for (s = first; *s; s++)
> > + for (s = first; *s;)
> > {
> > if (s == '\\')
> > {
> > - memmove (s, s+1, end-s-1);
> > - end--;
> > + / trailing backslash is not an escape character */
> > + if (s + 1 == end)
> > + break;
> > + memmove (s, s + 1, end - s);
> > + --end;
> > + ++s;
> > }
> > - if (isspace ((unsigned char) *s))
> > + else if (*s == '\'' || *s == '\"')
> > {
> > - if (sq || dq)
> > - continue;
> > - *s++ = '\0';
> > - while (isspace ((unsigned char) *s)) s++;
> > - if (*s)
> > - res[++i] = s;
> > - }
> > - if (*s == '\'' && !dq)
> > - {
> > - if (sq)
> > + if (!quote)
> > + quote = s++;
> > + else if (*s == *quote)
> > {
> > - memmove (sq, sq+1, s-sq-1);
> > - memmove (s-2, s+1, end-s-1);
> > + memmove (quote, quote + 1, s - quote - 1);
> > + memmove (s - 1, s + 1, end - s);
> > end -= 2;
> > - s--;
> > - sq = NULL;
> > + --s;
> > + quote = NULL;
> > }
> > else
> > - {
> > - sq = s;
> > - }
> > + ++s;
> > }
> > - if (*s == '"' && !sq)
> > + else if (!quote && isspace ((unsigned char) *s))
> > {
> > - if (dq)
> > - {
> > - memmove (dq, dq+1, s-dq-1);
> > - memmove (s-2, s+1, end-s-1);
> > - end -= 2;
> > - s--;
> > - dq = NULL;
> > - }
> > - else
> > - {
> > - dq = s;
> > - }
> > + *s++ = '\0';
> > + while (isspace ((unsigned char) *s))
> > + s++;
> > + if (*s)
> > + res[++i] = s;
> > }
> > + else
> > + ++s;
> > }
> > res[++i] = NULL;
> > return res;
> > --
> > 2.40.1
More information about the Binutils
mailing list