From: Szabolcs Nagy <nsz@port70.net>
To: Matthias Goergens <matthias.goergens@gmail.com>
Cc: musl@lists.openwall.com
Subject: Re: [musl] [PATCH] mntent: deal with escaped whitespace in mtab and fstab
Date: Wed, 29 Mar 2023 19:04:18 +0200 [thread overview]
Message-ID: <20230329170418.GD3630668@port70.net> (raw)
In-Reply-To: <20230329151751.392944-1-matthias.goergens@gmail.com>
* Matthias Goergens <matthias.goergens@gmail.com> [2023-03-29 23:17:51 +0800]:
> >From glibc's documentation:
>
> > Since fields in the mtab and fstab files are separated by whitespace,
> > octal escapes are used to represent the characters space (\040),
> > tab (\011), newline (\012), and backslash (\\) in those files when they
> > occur in one of the four strings in a mntent structure. The
> > routines addmntent() and getmntent() will convert from string
> > representation to escaped representation and back. When converting
> > from escaped representation, the sequence \134 is also converted to a
> > backslash.
>
> This fixes the issue reported in https://www.openwall.com/lists/musl/2021/12/14/1
>
> Please pardon the previous broken patch that I tried to send directly
> via gmail.
>
> This is my first time contributing to musl. Please point out any ways
> to improve. I probably got a few things wrong?
>
> Thanks!
>
> Addendum: this is a new version. The first one did not copy the final
> null-byte.
> ---
> src/misc/mntent.c | 75 ++++++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 68 insertions(+), 7 deletions(-)
>
> diff --git a/src/misc/mntent.c b/src/misc/mntent.c
> index d404fbe3..49f4e386 100644
> --- a/src/misc/mntent.c
> +++ b/src/misc/mntent.c
> @@ -1,3 +1,4 @@
> +#include <assert.h>
> #include <stdio.h>
> #include <string.h>
> #include <mntent.h>
> @@ -20,6 +21,37 @@ int endmntent(FILE *f)
> return 1;
> }
>
> +static inline int decode1(char** in_buf, char** out_buf, const char* from, const char to) {
> + if(strncmp(from, *in_buf, strlen(from)) == 0) {
> + *(*out_buf)++ = to;
> + *in_buf += strlen(from);
> + return 1;
> + }
> + return 0;
> +}
> +
> +static inline char* decode(char* buf) {
> + assert(buf != NULL);
> + char* read_cursor = buf;
> + char* write_cursor = buf;
> + while(*read_cursor) {
> + // space
> + decode1(&read_cursor, &write_cursor, "\\040", '\040')
> + // tab
> + || decode1(&read_cursor, &write_cursor, "\\011", '\011')
> + // newline
> + || decode1(&read_cursor, &write_cursor, "\\012", '\012')
> + // backslash
> + || decode1(&read_cursor, &write_cursor, "\\134", '\134')
> + || decode1(&read_cursor, &write_cursor, "\\\\", '\\')
> + // default: copy char as is.
> + || (*write_cursor++ = *read_cursor++);
> + }
this will try matching at every position, but
we know that each escape starts with \ so this
can be optimized.
i expect inlining decode1 will increase code size
unnecessarily. you can avoid that e.g. like
for (;;) {
...
const char *replace =
"\040" "040" "\0"
"\011" "011" "\0"
...
"\\" "\\" "\0"
"\\" "";
for (;;) {
char c = *replace++;
size_t n = strlen(replace);
if (strncmp(pr, replace, n) == 0) {
*pw++ = c;
pr += n;
break;
}
replace += n+1;
}
...
char *next = __strchrnul(pr, '\\');
memmove(pw, pr, next-pr);
...
}
> + *write_cursor = *read_cursor;
> +
> + return buf;
> +}
> +
> struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int buflen)
> {
> int n[8], use_internal = (linebuf == SENTINEL);
> @@ -55,10 +87,10 @@ struct mntent *getmntent_r(FILE *f, struct mntent *mnt, char *linebuf, int bufle
> linebuf[n[5]] = 0;
> linebuf[n[7]] = 0;
>
> - mnt->mnt_fsname = linebuf+n[0];
> - mnt->mnt_dir = linebuf+n[2];
> - mnt->mnt_type = linebuf+n[4];
> - mnt->mnt_opts = linebuf+n[6];
> + mnt->mnt_fsname = decode(linebuf+n[0]);
> + mnt->mnt_dir = decode(linebuf+n[2]);
> + mnt->mnt_type = decode(linebuf+n[4]);
> + mnt->mnt_opts = decode(linebuf+n[6]);
>
> return mnt;
> }
> @@ -69,12 +101,41 @@ struct mntent *getmntent(FILE *f)
> return getmntent_r(f, &mnt, SENTINEL, 0);
> }
>
> +static inline int write_string(FILE *f, const char* str)
> +{
> + char c;
> + int error_occured = 0;
> + while(str && !error_occured && (c = *str++) != 0) {
> + if(c == '\040') // space
> + error_occured = fprintf(f, "%s", "\\040") < 0;
> + else if (c == '\011') // tab
> + error_occured = fprintf(f, "%s", "\\011") < 0;
> + else if (c == '\012') // newline
> + error_occured = fprintf(f, "%s", "\\012") < 0;
> + else if (c == '\\')
> + error_occured = fprintf(f, "%s", "\\\\") < 0;
> + else
> + error_occured = fprintf(f, "%c", c) < 0;
> + }
> + return error_occured;
> +}
> +
> int addmntent(FILE *f, const struct mntent *mnt)
> {
> if (fseek(f, 0, SEEK_END)) return 1;
> - return fprintf(f, "%s\t%s\t%s\t%s\t%d\t%d\n",
> - mnt->mnt_fsname, mnt->mnt_dir, mnt->mnt_type, mnt->mnt_opts,
> - mnt->mnt_freq, mnt->mnt_passno) < 0;
the original code has a bug here: fprintf returns number of
chars printed but addmntent returns 0 to indicate success.
i think this should be fixed independently of the new feature.
(separate patch).
> + flockfile(f);
> + int result =
> + write_string(f, mnt->mnt_fsname)
> + || (fprintf(f, "\t") < 0)
> + || write_string(f, mnt->mnt_dir)
> + || (fprintf(f, "\t") < 0)
> + || write_string(f, mnt->mnt_type)
> + || (fprintf(f, "\t") < 0)
> + || write_string(f, mnt->mnt_opts)
> + || (fprintf(f, "\t%d\t%d\n",
> + mnt->mnt_freq, mnt->mnt_passno) < 0);
> + funlockfile(f);
this looks a bit ugly (recursive locks and char-by-char
printf) but i don't see anything wrong (again i would not
use inline).
> + return result;
> }
>
> char *hasmntopt(const struct mntent *mnt, const char *opt)
> --
> 2.40.0
next prev parent reply other threads:[~2023-03-29 17:11 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-29 15:17 Matthias Goergens
2023-03-29 17:04 ` Szabolcs Nagy [this message]
2023-03-29 17:16 ` Szabolcs Nagy
2023-03-30 8:14 ` Matthias Görgens
2023-03-30 9:29 ` Pascal Cuoq
2023-03-30 13:53 ` Rich Felker
-- strict thread matches above, loose matches on Subject: below --
2023-03-30 9:23 Matthias Goergens
2023-03-29 8:46 Matthias Goergens
2023-03-29 8:38 Matthias Görgens
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230329170418.GD3630668@port70.net \
--to=nsz@port70.net \
--cc=matthias.goergens@gmail.com \
--cc=musl@lists.openwall.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.vuxu.org/mirror/musl/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).