From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-1.0 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_MSPIKE_H2 autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 25639 invoked from network); 29 Mar 2023 17:11:17 -0000 Received: from second.openwall.net (193.110.157.125) by inbox.vuxu.org with ESMTPUTF8; 29 Mar 2023 17:11:17 -0000 Received: (qmail 32448 invoked by uid 550); 29 Mar 2023 17:11:14 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 32408 invoked from network); 29 Mar 2023 17:11:13 -0000 Date: Wed, 29 Mar 2023 19:04:18 +0200 From: Szabolcs Nagy To: Matthias Goergens Cc: musl@lists.openwall.com Message-ID: <20230329170418.GD3630668@port70.net> Mail-Followup-To: Matthias Goergens , musl@lists.openwall.com References: <20230329151751.392944-1-matthias.goergens@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230329151751.392944-1-matthias.goergens@gmail.com> Subject: Re: [musl] [PATCH] mntent: deal with escaped whitespace in mtab and fstab * Matthias Goergens [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 > #include > #include > #include > @@ -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