* Implementation of GLOB_TILDE
@ 2017-01-17 15:44 Bernardo Pascoal Figueiredo
2017-01-17 15:49 ` Bernardo Pascoal Figueiredo
0 siblings, 1 reply; 6+ messages in thread
From: Bernardo Pascoal Figueiredo @ 2017-01-17 15:44 UTC (permalink / raw)
To: musl
[-- Attachment #1: Type: text/plain, Size: 1001 bytes --]
Hi,
I'm sending as attachment a a patch that implments the GLOB_TILDE
extension to
the glob function.
This functions is helpful to compile some software that otherwise would
not
compile(without patching) with musl like i3wm.
I tested the code(test file as attachment) and the results seem to be
correct.
I have a few questions though:
* How do I contribute to musl? Should I just send patches to this
mailing list
or should I make pull requests?
* I defined GLOB_TILDE as 0x100, but I think this won't work on
architectures
that have sizeof(int) == 2, as the flags argument in glob is an int.
* I think it's best to define GLOB_TILDE in glob.h inside a '#if
defined(_GNU_SOURCE) || defined(_BSD_SOURCE)' what do you think?
* I had to copy strlcat and strlcpy to glob.c so I could use them. I
had to do
this because musl isn't compile as _GNU_SOURCE or _BSD_SOURCE so
string.h
doesn't expose these functions. How should I fix this?
Thanks,
Bernardo Figueiredo
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: test.c --]
[-- Type: text/x-c; name=test.c, Size: 581 bytes --]
#include <glob.h>
#include <stdbool.h>
#include <stdio.h>
int no_error(const char *p, int e) {
return 0;
}
int test_glob(const char *p) {
glob_t g;
int result = false;
result = glob(p, GLOB_TILDE, no_error, &g);
printf("result:%d", result);
if(result == 0) {
for(int i = 0; i < g.gl_pathc; ++i) {
printf(" '%s'", g.gl_pathv[i]);
}
}
printf("\n");
}
int main(int argc, char **argv) {
(void) argc; (void) argv;
test_glob("~");
test_glob("~/p*");
test_glob("~bpf");
test_glob("~bpf/p*");
return 0;
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Implementation of GLOB_TILDE
2017-01-17 15:44 Implementation of GLOB_TILDE Bernardo Pascoal Figueiredo
@ 2017-01-17 15:49 ` Bernardo Pascoal Figueiredo
2017-01-17 19:39 ` Rich Felker
0 siblings, 1 reply; 6+ messages in thread
From: Bernardo Pascoal Figueiredo @ 2017-01-17 15:49 UTC (permalink / raw)
To: musl
[-- Attachment #1: Type: text/plain, Size: 1138 bytes --]
Forgot the patch file.
On 2017-01-17 15:44, Bernardo Pascoal Figueiredo wrote:
> Hi,
>
> I'm sending as attachment a a patch that implments the GLOB_TILDE
> extension to
> the glob function.
>
> This functions is helpful to compile some software that otherwise would
> not
> compile(without patching) with musl like i3wm.
>
> I tested the code(test file as attachment) and the results seem to be
> correct.
>
> I have a few questions though:
> * How do I contribute to musl? Should I just send patches to this
> mailing list
> or should I make pull requests?
>
> * I defined GLOB_TILDE as 0x100, but I think this won't work on
> architectures
> that have sizeof(int) == 2, as the flags argument in glob is an int.
>
> * I think it's best to define GLOB_TILDE in glob.h inside a '#if
> defined(_GNU_SOURCE) || defined(_BSD_SOURCE)' what do you think?
>
> * I had to copy strlcat and strlcpy to glob.c so I could use them. I
> had to do
> this because musl isn't compile as _GNU_SOURCE or _BSD_SOURCE so
> string.h
> doesn't expose these functions. How should I fix this?
>
> Thanks,
> Bernardo Figueiredo
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: glob_tilde.patch --]
[-- Type: text/x-diff; name=glob_tilde.patch, Size: 4516 bytes --]
diff --git a/include/glob.h b/include/glob.h
index 76f6c1c6..fc8106b2 100644
--- a/include/glob.h
+++ b/include/glob.h
@@ -30,6 +30,7 @@ void globfree(glob_t *);
#define GLOB_APPEND 0x20
#define GLOB_NOESCAPE 0x40
#define GLOB_PERIOD 0x80
+#define GLOB_TILDE 0x100
#define GLOB_NOSPACE 1
#define GLOB_ABORTED 2
diff --git a/src/regex/glob.c b/src/regex/glob.c
index 5b6ff124..f40da380 100644
--- a/src/regex/glob.c
+++ b/src/regex/glob.c
@@ -8,6 +8,9 @@
#include <errno.h>
#include <stddef.h>
#include "libc.h"
+#include <stdbool.h>
+#include <pwd.h>
+#include <unistd.h>
struct match
{
@@ -154,13 +157,124 @@ static int sort(const void *a, const void *b)
return strcmp(*(const char **)a, *(const char **)b);
}
+#include <stdint.h>
+#define GLOB_STRLCPY_ALIGN (sizeof(size_t)-1)
+#define GLOB_STRLCPY_ONES ((size_t)-1/UCHAR_MAX)
+#define GLOB_STRLCPY_HIGHS (GLOB_STRLCPY_ONES * (UCHAR_MAX/2+1))
+#define GLOB_STRLCPY_HASZERO(x) ((x)-GLOB_STRLCPY_ONES & ~(x) & GLOB_STRLCPY_HIGHS)
+
+size_t glob_strlcpy(char *d, const char *s, size_t n)
+{
+ char *d0 = d;
+ size_t *wd;
+ const size_t *ws;
+
+ if (!n--) goto finish;
+ if (((uintptr_t)s & GLOB_STRLCPY_ALIGN) == ((uintptr_t)d & GLOB_STRLCPY_ALIGN)) {
+ for (; ((uintptr_t)s & GLOB_STRLCPY_ALIGN) && n && (*d=*s); n--, s++, d++);
+ if (n && *s) {
+ wd=(void *)d; ws=(const void *)s;
+ for (; n>=sizeof(size_t) && !GLOB_STRLCPY_HASZERO(*ws);
+ n-=sizeof(size_t), ws++, wd++) *wd = *ws;
+ d=(void *)wd; s=(const void *)ws;
+ }
+ }
+ for (; n && (*d=*s); n--, s++, d++);
+ *d = 0;
+finish:
+ return d-d0 + strlen(s);
+}
+
+static size_t glob_strlcat(char *d, const char *s, size_t n)
+{
+ size_t l = strnlen(d, n);
+ if (l == n) return l + strlen(s);
+ return l + glob_strlcpy(d+l, s, n-l);
+}
+
+
+
+/*"~" or "~/(...)" case*/
+static bool expand_tilde_cur_user(const char *pat_after_tilde, char *new_pat, size_t new_pat_size)
+{
+ char *home;
+ struct passwd pw_store, *pw_result;
+ char pw_buf[1024];
+
+ /*FIXME: add check for issetugid as in libc of openbsd?*/
+ home = getenv("HOME");
+ if(home == NULL) {
+ getpwuid_r(getuid(), &pw_store, pw_buf, sizeof(pw_buf), &pw_result);
+ if(pw_result == NULL) {
+ return false;
+ }
+ home = pw_store.pw_dir;
+ }
+
+ return glob_strlcpy(new_pat, home, new_pat_size) < new_pat_size
+ && glob_strlcat(new_pat, pat_after_tilde, new_pat_size) < new_pat_size;
+}
+
+/* "~user/(...) case*/
+static bool expand_tilde_named_user(const char *pat_after_tilde, char *new_pat, size_t new_pat_size)
+{
+ struct passwd pw_store, *pw_result;
+ char pw_buf[1024], username[1024];
+ const char *slash_pos = strchr(pat_after_tilde, '/');
+ if(slash_pos == NULL) {
+ return false;
+ }
+
+ ptrdiff_t pat_username_size = slash_pos - pat_after_tilde;
+ if(pat_username_size <= 0 || pat_username_size >= sizeof(username)) {
+ return false;
+ }
+ strncpy(username, pat_after_tilde, pat_username_size);
+ username[pat_username_size] = '\0';
+
+ getpwnam_r(username, &pw_store, pw_buf, sizeof(pw_buf), &pw_result);
+ if (pw_result == NULL)
+ return false;
+
+ return glob_strlcpy(new_pat, pw_store.pw_dir, new_pat_size) < new_pat_size
+ && glob_strlcat(new_pat, slash_pos, new_pat_size) < new_pat_size;
+}
+
+/* expands:
+ * ~ into /home/user/
+ * ~/asd into /home/user/asd
+ * ~user1/asd into /home/user1/asd
+ * the values for the home directory are taken from passwd
+ *
+ * returning true means successful expansion and that expanded_pat is valid
+ */
+static bool expand_tilde(const char *pat, char *new_pat, int new_pat_size)
+{
+ const char *pat_after_tilde = pat + 1;
+ if(*pat_after_tilde == '\0' || *pat_after_tilde == '/') {
+ return expand_tilde_cur_user(pat_after_tilde, new_pat, new_pat_size);
+ } else {
+ return expand_tilde_named_user(pat_after_tilde, new_pat, new_pat_size);
+ }
+}
+
int glob(const char *restrict pat, int flags, int (*errfunc)(const char *path, int err), glob_t *restrict g)
{
- const char *p=pat, *d;
+ const char *p, *d;
+ char new_pat[PATH_MAX + 1];
struct match head = { .next = NULL }, *tail = &head;
size_t cnt, i;
size_t offs = (flags & GLOB_DOOFFS) ? g->gl_offs : 0;
int error = 0;
+
+ /*even if expanding fails(e.g. expansion make pat too big)
+ * we should try to match the ~ or ~user literally*/
+ bool should_expand_tilde = (flags & GLOB_TILDE) && (pat[0] == '~');
+ if(should_expand_tilde && expand_tilde(pat, new_pat, sizeof(new_pat))) {
+ p = new_pat;
+ } else {
+ p = pat;
+ }
if (*p == '/') {
for (; *p == '/'; p++);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Re: Implementation of GLOB_TILDE
2017-01-17 15:49 ` Bernardo Pascoal Figueiredo
@ 2017-01-17 19:39 ` Rich Felker
2017-01-18 19:51 ` Bernardo Pascoal Figueiredo
0 siblings, 1 reply; 6+ messages in thread
From: Rich Felker @ 2017-01-17 19:39 UTC (permalink / raw)
To: musl
On Tue, Jan 17, 2017 at 03:49:03PM +0000, Bernardo Pascoal Figueiredo wrote:
> >I have a few questions though:
> > * How do I contribute to musl? Should I just send patches to this
> >mailing list
This is the preferred way, yes.
> > * I defined GLOB_TILDE as 0x100, but I think this won't work on
> >architectures
> > that have sizeof(int) == 2, as the flags argument in glob is an int.
That's not an issue. (1) POSIX requires >=32bit int; musl is even more
restrictive. (2) 0xff is 8 bits, not 16. Even ISO C requires 0x100 to
fit in int. You do need to match whatever value glibc uses, though; I
haven't checked that.
> > * I think it's best to define GLOB_TILDE in glob.h inside a '#if
> > defined(_GNU_SOURCE) || defined(_BSD_SOURCE)' what do you think?
> >
> > * I had to copy strlcat and strlcpy to glob.c so I could use
> >them. I had to do
> > this because musl isn't compile as _GNU_SOURCE or _BSD_SOURCE
> >so string.h
> > doesn't expose these functions. How should I fix this?
Just don't use them. The same thing can be achieved much better with
portable functions strnlen and memcpy.
Note that even if you added #define for _GNU_SOURCE to this file, you
couldn't reference these functions because then a function in the
standard namespace would depend on symbols in nonstandard namespace.
> diff --git a/src/regex/glob.c b/src/regex/glob.c
> index 5b6ff124..f40da380 100644
> --- a/src/regex/glob.c
> +++ b/src/regex/glob.c
> @@ -8,6 +8,9 @@
> #include <errno.h>
> #include <stddef.h>
> #include "libc.h"
> +#include <stdbool.h>
Just use int for boolean values; bool is not idiomatic in musl.
> +/*"~" or "~/(...)" case*/
> +static bool expand_tilde_cur_user(const char *pat_after_tilde, char *new_pat, size_t new_pat_size)
> +{
> + char *home;
> + struct passwd pw_store, *pw_result;
> + char pw_buf[1024];
> +
> + /*FIXME: add check for issetugid as in libc of openbsd?*/
> + home = getenv("HOME");
> + if(home == NULL) {
> + getpwuid_r(getuid(), &pw_store, pw_buf, sizeof(pw_buf), &pw_result);
> + if(pw_result == NULL) {
> + return false;
> + }
> + home = pw_store.pw_dir;
> + }
> +
> + return glob_strlcpy(new_pat, home, new_pat_size) < new_pat_size
> + && glob_strlcat(new_pat, pat_after_tilde, new_pat_size) < new_pat_size;
> +}
> +
> +/* "~user/(...) case*/
> +static bool expand_tilde_named_user(const char *pat_after_tilde, char *new_pat, size_t new_pat_size)
> +{
> + struct passwd pw_store, *pw_result;
> + char pw_buf[1024], username[1024];
> + const char *slash_pos = strchr(pat_after_tilde, '/');
> + if(slash_pos == NULL) {
> + return false;
> + }
> +
> + ptrdiff_t pat_username_size = slash_pos - pat_after_tilde;
> + if(pat_username_size <= 0 || pat_username_size >= sizeof(username)) {
> + return false;
> + }
> + strncpy(username, pat_after_tilde, pat_username_size);
> + username[pat_username_size] = '\0';
> +
> + getpwnam_r(username, &pw_store, pw_buf, sizeof(pw_buf), &pw_result);
> + if (pw_result == NULL)
> + return false;
> +
> + return glob_strlcpy(new_pat, pw_store.pw_dir, new_pat_size) < new_pat_size
> + && glob_strlcat(new_pat, slash_pos, new_pat_size) < new_pat_size;
> +}
It should be possible to reduce this code considerably, and to avoid
some of the large stack buffers. Certainly username[] does not need to
be 1k; I believe there's a macro for the max supported in limits.h or
such and it's something like 16 or 32 bytes.
> int glob(const char *restrict pat, int flags, int (*errfunc)(const char *path, int err), glob_t *restrict g)
> {
> - const char *p=pat, *d;
> + const char *p, *d;
> + char new_pat[PATH_MAX + 1];
> struct match head = { .next = NULL }, *tail = &head;
> size_t cnt, i;
> size_t offs = (flags & GLOB_DOOFFS) ? g->gl_offs : 0;
> int error = 0;
> +
> + /*even if expanding fails(e.g. expansion make pat too big)
> + * we should try to match the ~ or ~user literally*/
> + bool should_expand_tilde = (flags & GLOB_TILDE) && (pat[0] == '~');
> + if(should_expand_tilde && expand_tilde(pat, new_pat, sizeof(new_pat))) {
> + p = new_pat;
> + } else {
> + p = pat;
> + }
Don't introduce gratuitous variables for expressions used just once.
The conditions going into that bool can just be part of the if.
I haven't checked what happens when the input pat is too long to fit
in new_pat. This needs to be an error condition, not silent
truncation, but error conditions can't be handled until further down;
see commit 769f53598e781ffc89191520f3f8a93cb58db91f for why. Also,
new_pat should probably be a VLA whose length is 1 unless tilde
expansion is needed, and PATH_MAX otherwise, so that glob doesn't blow
an extra page on the stack when tilde expansion is not in use.
Rich
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Re: Implementation of GLOB_TILDE
2017-01-17 19:39 ` Rich Felker
@ 2017-01-18 19:51 ` Bernardo Pascoal Figueiredo
2017-01-20 9:56 ` Bernardo Pascoal Figueiredo
0 siblings, 1 reply; 6+ messages in thread
From: Bernardo Pascoal Figueiredo @ 2017-01-18 19:51 UTC (permalink / raw)
To: musl
[-- Attachment #1: Type: text/plain, Size: 8088 bytes --]
On 2017-01-17 19:39, Rich Felker wrote:
> On Tue, Jan 17, 2017 at 03:49:03PM +0000, Bernardo Pascoal Figueiredo
> wrote:
>> >I have a few questions though:
>> > * How do I contribute to musl? Should I just send patches to this
>> >mailing list
>
> This is the preferred way, yes.
>
>> > * I defined GLOB_TILDE as 0x100, but I think this won't work on
>> >architectures
>> > that have sizeof(int) == 2, as the flags argument in glob is an int.
>
> That's not an issue. (1) POSIX requires >=32bit int; musl is even more
> restrictive. (2) 0xff is 8 bits, not 16. Even ISO C requires 0x100 to
> fit in int. You do need to match whatever value glibc uses, though; I
> haven't checked that.
They use 1 << 12 which is 0x1000. I fixed this in the patch below.
Does this really matter? Is musl supposed to be binary compatible with
glibc?
OpenBSD's libc uses 0x800 for example.
>
>> > * I think it's best to define GLOB_TILDE in glob.h inside a '#if
>> > defined(_GNU_SOURCE) || defined(_BSD_SOURCE)' what do you think?
>> >
>> > * I had to copy strlcat and strlcpy to glob.c so I could use
>> >them. I had to do
>> > this because musl isn't compile as _GNU_SOURCE or _BSD_SOURCE
>> >so string.h
>> > doesn't expose these functions. How should I fix this?
>
> Just don't use them. The same thing can be achieved much better with
> portable functions strnlen and memcpy.
>
> Note that even if you added #define for _GNU_SOURCE to this file, you
> couldn't reference these functions because then a function in the
> standard namespace would depend on symbols in nonstandard namespace.
>
What I was thinking was to have private musl implementations of strlcat
and
strlcpy and have the public strlcat and strlcpy call these private ones.
That way it'd be possible to use strlcat and strlcpy within musl.
I'd prefer that to strnlen and memcpy because that's what strlcat and
strlcpy
already do, so I'd be duplicating it's functionality. This is way more
error
prone.
>> diff --git a/src/regex/glob.c b/src/regex/glob.c
>> index 5b6ff124..f40da380 100644
>> --- a/src/regex/glob.c
>> +++ b/src/regex/glob.c
>> @@ -8,6 +8,9 @@
>> #include <errno.h>
>> #include <stddef.h>
>> #include "libc.h"
>> +#include <stdbool.h>
>
> Just use int for boolean values; bool is not idiomatic in musl.
Fixed
>
>> +/*"~" or "~/(...)" case*/
>> +static bool expand_tilde_cur_user(const char *pat_after_tilde, char
>> *new_pat, size_t new_pat_size)
>> +{
>> + char *home;
>> + struct passwd pw_store, *pw_result;
>> + char pw_buf[1024];
>> +
>> + /*FIXME: add check for issetugid as in libc of openbsd?*/
>> + home = getenv("HOME");
>> + if(home == NULL) {
>> + getpwuid_r(getuid(), &pw_store, pw_buf, sizeof(pw_buf),
>> &pw_result);
>> + if(pw_result == NULL) {
>> + return false;
>> + }
>> + home = pw_store.pw_dir;
>> + }
>> +
>> + return glob_strlcpy(new_pat, home, new_pat_size) < new_pat_size
>> + && glob_strlcat(new_pat, pat_after_tilde, new_pat_size) <
>> new_pat_size;
>> +}
>> +
>> +/* "~user/(...) case*/
>> +static bool expand_tilde_named_user(const char *pat_after_tilde, char
>> *new_pat, size_t new_pat_size)
>> +{
>> + struct passwd pw_store, *pw_result;
>> + char pw_buf[1024], username[1024];
>> + const char *slash_pos = strchr(pat_after_tilde, '/');
>> + if(slash_pos == NULL) {
>> + return false;
>> + }
>> +
>> + ptrdiff_t pat_username_size = slash_pos - pat_after_tilde;
>> + if(pat_username_size <= 0 || pat_username_size >=
>> sizeof(username)) {
>> + return false;
>> + }
>> + strncpy(username, pat_after_tilde, pat_username_size);
>> + username[pat_username_size] = '\0';
>> +
>> + getpwnam_r(username, &pw_store, pw_buf, sizeof(pw_buf),
>> &pw_result);
>> + if (pw_result == NULL)
>> + return false;
>> +
>> + return glob_strlcpy(new_pat, pw_store.pw_dir, new_pat_size) <
>> new_pat_size
>> + && glob_strlcat(new_pat, slash_pos, new_pat_size) <
>> new_pat_size;
>> +}
>
> It should be possible to reduce this code considerably, and to avoid
> some of the large stack buffers. Certainly username[] does not need to
> be 1k; I believe there's a macro for the max supported in limits.h or
> such and it's something like 16 or 32 bytes.
I searched and found there is a define for LOGIN_NAME_MAX which is 256.
According to the useradd man page the maximum number of characters of an
username is 32.
There is also in limits.h _POSIX_NAME_MAX which is 14 and
_POSIX_LOGIN_NAME_MAX
which is 9.
The openbsd pwd.h has a define for _PW_NAME_LEN which is 31 (without
'\0').
Which alternative is the best?
(I updated the username arrays to be LOGIN_NAME_MAX)
Relative to the pw_buf I don't think musl's code has any variable that
defines
the maximum supported passwd line.
I used 1024 because that's what I saw in the openbsd code. They have a
global
char [] of size _PW_BUF_LEN which is 1024.
Reduce the code in which way? Try to refactor some code of
expand_tilde_cur_user and expand_tilde_named_user, of make the code
"less
verbose"?
>
>> int glob(const char *restrict pat, int flags, int (*errfunc)(const
>> char *path, int err), glob_t *restrict g)
>> {
>> - const char *p=pat, *d;
>> + const char *p, *d;
>> + char new_pat[PATH_MAX + 1];
>> struct match head = { .next = NULL }, *tail = &head;
>> size_t cnt, i;
>> size_t offs = (flags & GLOB_DOOFFS) ? g->gl_offs : 0;
>> int error = 0;
>> +
>> + /*even if expanding fails(e.g. expansion make pat too big)
>> + * we should try to match the ~ or ~user literally*/
>> + bool should_expand_tilde = (flags & GLOB_TILDE) && (pat[0] ==
>> '~');
>> + if(should_expand_tilde && expand_tilde(pat, new_pat,
>> sizeof(new_pat))) {
>> + p = new_pat;
>> + } else {
>> + p = pat;
>> + }
>
> Don't introduce gratuitous variables for expressions used just once.
> The conditions going into that bool can just be part of the if.
If I put it inside the if the line would be too big and those two checks
go
together.
I think it's alot clearer.
>
> I haven't checked what happens when the input pat is too long to fit
> in new_pat. This needs to be an error condition, not silent
> truncation, but error conditions can't be handled until further down;
> see commit 769f53598e781ffc89191520f3f8a93cb58db91f for why. Also,
> new_pat should probably be a VLA whose length is 1 unless tilde
> expansion is needed, and PATH_MAX otherwise, so that glob doesn't blow
> an extra page on the stack when tilde expansion is not in use.
I changed it to char new_pat[tilde_flag ? PATH_MAX : 1] where tilde_flag
=
flags & GLOB_TILDE.
(new_pat no longer uses an extra page.)
Alternatively I can put most of the code of glob in another function and
only
create a new pat buffer when we should try to expand the tilde.
This would be good to reduce the scope of new_pat.
When the expanded pattern is too big to fit new_pat, the expansion fails
and
the unexpanded pattern is used.
According to glibc GLOB_TILDE should never make glob return an error. If
anything related to the expansion of tilde fails it's supposed to
continue glob
with the pattern unexpanded.
At least that how I interpret the manual:
"
If the username is invalid, or the home directory cannot be determined,
then no
substitution is performed.
"
What I'm trying to say is that GLOB_TILDE never produces an error, so
the code
that zeros the glob_t structure always runs.
If the code that checks for leading '/' is put just before the call to
match_dir, it'd be ok to put the code that calls expand_tilde after the
glob_t
zeroing and strlen checking and before the leading '/' code.
The "downside" of this is that the caller of glob has a ton of leading
'/' it
could now fail the strlen check, but I think that's the fault of who is
calling
glob with "/////////////////////(...)"
A patch for that is in attachment "move_leading_slash_check_down.patch".
>
> Rich
I also send the patch with the things I fixed as attachment. It's still
not okay.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: glob_tilde.patch --]
[-- Type: text/x-diff; name=glob_tilde.patch, Size: 4543 bytes --]
diff --git a/include/glob.h b/include/glob.h
index 76f6c1c6..477ddf2b 100644
--- a/include/glob.h
+++ b/include/glob.h
@@ -30,6 +30,7 @@ void globfree(glob_t *);
#define GLOB_APPEND 0x20
#define GLOB_NOESCAPE 0x40
#define GLOB_PERIOD 0x80
+#define GLOB_TILDE 0x1000
#define GLOB_NOSPACE 1
#define GLOB_ABORTED 2
diff --git a/src/regex/glob.c b/src/regex/glob.c
index 5b6ff124..37bbfafd 100644
--- a/src/regex/glob.c
+++ b/src/regex/glob.c
@@ -8,6 +8,8 @@
#include <errno.h>
#include <stddef.h>
#include "libc.h"
+#include <pwd.h>
+#include <unistd.h>
struct match
{
@@ -154,13 +156,125 @@ static int sort(const void *a, const void *b)
return strcmp(*(const char **)a, *(const char **)b);
}
+#include <stdint.h>
+#define GLOB_STRLCPY_ALIGN (sizeof(size_t)-1)
+#define GLOB_STRLCPY_ONES ((size_t)-1/UCHAR_MAX)
+#define GLOB_STRLCPY_HIGHS (GLOB_STRLCPY_ONES * (UCHAR_MAX/2+1))
+#define GLOB_STRLCPY_HASZERO(x) ((x)-GLOB_STRLCPY_ONES & ~(x) & GLOB_STRLCPY_HIGHS)
+
+size_t glob_strlcpy(char *d, const char *s, size_t n)
+{
+ char *d0 = d;
+ size_t *wd;
+ const size_t *ws;
+
+ if (!n--) goto finish;
+ if (((uintptr_t)s & GLOB_STRLCPY_ALIGN) == ((uintptr_t)d & GLOB_STRLCPY_ALIGN)) {
+ for (; ((uintptr_t)s & GLOB_STRLCPY_ALIGN) && n && (*d=*s); n--, s++, d++);
+ if (n && *s) {
+ wd=(void *)d; ws=(const void *)s;
+ for (; n>=sizeof(size_t) && !GLOB_STRLCPY_HASZERO(*ws);
+ n-=sizeof(size_t), ws++, wd++) *wd = *ws;
+ d=(void *)wd; s=(const void *)ws;
+ }
+ }
+ for (; n && (*d=*s); n--, s++, d++);
+ *d = 0;
+finish:
+ return d-d0 + strlen(s);
+}
+
+static size_t glob_strlcat(char *d, const char *s, size_t n)
+{
+ size_t l = strnlen(d, n);
+ if (l == n) return l + strlen(s);
+ return l + glob_strlcpy(d+l, s, n-l);
+}
+
+
+
+/*"~" or "~/(...)" case*/
+static int expand_tilde_cur_user(const char *pat_after_tilde, char *new_pat, size_t new_pat_size)
+{
+ char *home;
+ struct passwd pw_store, *pw_result;
+ char pw_buf[LOGIN_NAME_MAX];
+
+ /*FIXME: add check for issetugid as in libc of openbsd?*/
+ home = getenv("HOME");
+ if(home == NULL) {
+ getpwuid_r(getuid(), &pw_store, pw_buf, sizeof(pw_buf), &pw_result);
+ if(pw_result == NULL) {
+ return 0;
+ }
+ home = pw_store.pw_dir;
+ }
+
+ return glob_strlcpy(new_pat, home, new_pat_size) < new_pat_size
+ && glob_strlcat(new_pat, pat_after_tilde, new_pat_size) < new_pat_size;
+}
+
+/* "~user/(...) case*/
+static int expand_tilde_named_user(const char *pat_after_tilde, char *new_pat, size_t new_pat_size)
+{
+ struct passwd pw_store, *pw_result;
+ char pw_buf[1024], username[LOGIN_NAME_MAX];
+ const char *slash_pos = strchr(pat_after_tilde, '/');
+ if(slash_pos == NULL) {
+ return 0;
+ }
+
+ ptrdiff_t pat_username_size = slash_pos - pat_after_tilde;
+ if(pat_username_size <= 0 || pat_username_size >= sizeof(username)) {
+ return 0;
+ }
+ strncpy(username, pat_after_tilde, pat_username_size);
+ username[pat_username_size] = '\0';
+
+ getpwnam_r(username, &pw_store, pw_buf, sizeof(pw_buf), &pw_result);
+ if (pw_result == NULL)
+ return 0;
+
+ return glob_strlcpy(new_pat, pw_store.pw_dir, new_pat_size) < new_pat_size
+ && glob_strlcat(new_pat, slash_pos, new_pat_size) < new_pat_size;
+}
+
+/* expands:
+ * ~ into /home/user/
+ * ~/asd into /home/user/asd
+ * ~user1/asd into /home/user1/asd
+ * the values for the home directory are taken from passwd
+ *
+ * returning true means successful expansion and that expanded_pat is valid
+ */
+static int expand_tilde(const char *pat, char *new_pat, int new_pat_size)
+{
+ const char *pat_after_tilde = pat + 1;
+ if(*pat_after_tilde == '\0' || *pat_after_tilde == '/') {
+ return expand_tilde_cur_user(pat_after_tilde, new_pat, new_pat_size);
+ } else {
+ return expand_tilde_named_user(pat_after_tilde, new_pat, new_pat_size);
+ }
+}
+
int glob(const char *restrict pat, int flags, int (*errfunc)(const char *path, int err), glob_t *restrict g)
{
- const char *p=pat, *d;
+ const char *p, *d;
+ const int tilde_flag = flags & GLOB_TILDE;
+ char new_pat[tilde_flag ? PATH_MAX : 1];
struct match head = { .next = NULL }, *tail = &head;
size_t cnt, i;
size_t offs = (flags & GLOB_DOOFFS) ? g->gl_offs : 0;
int error = 0;
+
+ /*even if expanding fails(e.g. expansion make pat too big)
+ * we should try to match the ~ or ~user literally*/
+ int should_expand_tilde = tilde_flag && (pat[0] == '~');
+ if(should_expand_tilde && expand_tilde(pat, new_pat, sizeof(new_pat))) {
+ p = new_pat;
+ } else {
+ p = pat;
+ }
if (*p == '/') {
for (; *p == '/'; p++);
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: move_leading_slash_check_down.patch --]
[-- Type: text/x-diff; name=move_leading_slash_check_down.patch, Size: 829 bytes --]
diff --git a/src/regex/glob.c b/src/regex/glob.c
index 5b6ff124..cc9e012f 100644
--- a/src/regex/glob.c
+++ b/src/regex/glob.c
@@ -162,13 +162,6 @@ int glob(const char *restrict pat, int flags, int (*errfunc)(const char *path, i
size_t offs = (flags & GLOB_DOOFFS) ? g->gl_offs : 0;
int error = 0;
- if (*p == '/') {
- for (; *p == '/'; p++);
- d = "/";
- } else {
- d = "";
- }
-
if (!errfunc) errfunc = ignore_err;
if (!(flags & GLOB_APPEND)) {
@@ -179,6 +172,13 @@ int glob(const char *restrict pat, int flags, int (*errfunc)(const char *path, i
if (strnlen(p, PATH_MAX+1) > PATH_MAX) return GLOB_NOSPACE;
+ if (*p == '/') {
+ for (; *p == '/'; p++);
+ d = "/";
+ } else {
+ d = "";
+ }
+
if (*p) error = match_in_dir(d, p, flags, errfunc, &tail);
if (error == GLOB_NOSPACE) {
freelist(&head);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Re: Implementation of GLOB_TILDE
2017-01-18 19:51 ` Bernardo Pascoal Figueiredo
@ 2017-01-20 9:56 ` Bernardo Pascoal Figueiredo
0 siblings, 0 replies; 6+ messages in thread
From: Bernardo Pascoal Figueiredo @ 2017-01-20 9:56 UTC (permalink / raw)
To: musl
[-- Attachment #1: Type: text/plain, Size: 8655 bytes --]
Sending a new patch without strlcpy and strlcat.
Thanks for the help on IRC.
On 2017-01-18 19:51, Bernardo Pascoal Figueiredo wrote:
> On 2017-01-17 19:39, Rich Felker wrote:
>> On Tue, Jan 17, 2017 at 03:49:03PM +0000, Bernardo Pascoal Figueiredo
>> wrote:
>>> >I have a few questions though:
>>> > * How do I contribute to musl? Should I just send patches to this
>>> >mailing list
>>
>> This is the preferred way, yes.
>>
>>> > * I defined GLOB_TILDE as 0x100, but I think this won't work on
>>> >architectureshttps://webmail.tecnico.ulisboa.pt/rc/?_task=mail&_action=compose&_id=5497761105881de774e18d#
>>> > that have sizeof(int) == 2, as the flags argument in glob is an int.
>>
>> That's not an issue. (1) POSIX requires >=32bit int; musl is even more
>> restrictive. (2) 0xff is 8 bits, not 16. Even ISO C requires 0x100 to
>> fit in int. You do need to match whatever value glibc uses, though; I
>> haven't checked that.
>
> They use 1 << 12 which is 0x1000. I fixed this in the patch below.
> Does this really matter? Is musl supposed to be binary compatible with
> glibc?
> OpenBSD's libc uses 0x800 for example.
>
>>
>>> > * I think it's best to define GLOB_TILDE in glob.h inside a '#if
>>> > defined(_GNU_SOURCE) || defined(_BSD_SOURCE)' what do you think?
>>> >
>>> > * I had to copy strlcat and strlcpy to glob.c so I could use
>>> >them. I had to do
>>> > this because musl isn't compile as _GNU_SOURCE or _BSD_SOURCE
>>> >so string.h
>>> > doesn't expose these functions. How should I fix this?
>>
>> Just don't use them. The same thing can be achieved much better with
>> portable functions strnlen and memcpy.
>>
>> Note that even if you added #define for _GNU_SOURCE to this file, you
>> couldn't reference these functions because then a function in the
>> standard namespace would depend on symbols in nonstandard namespace.
>>
>
> What I was thinking was to have private musl implementations of strlcat
> and
> strlcpy and have the public strlcat and strlcpy call these private
> ones.
> That way it'd be possible to use strlcat and strlcpy within musl.
> I'd prefer that to strnlen and memcpy because that's what strlcat and
> strlcpy
> already do, so I'd be duplicating it's functionality. This is way more
> error
> prone.
>
>>> diff --git a/src/regex/glob.c b/src/regex/glob.c
>>> index 5b6ff124..f40da380 100644
>>> --- a/src/regex/glob.c
>>> +++ b/src/regex/glob.c
>>> @@ -8,6 +8,9 @@
>>> #include <errno.h>
>>> #include <stddef.h>
>>> #include "libc.h"
>>> +#include <stdbool.h>
>>
>> Just use int for boolean values; bool is not idiomatic in musl.
>
> Fixed
>
>>
>>> +/*"~" or "~/(...)" case*/
>>> +static bool expand_tilde_cur_user(const char *pat_after_tilde, char
>>> *new_pat, size_t new_pat_size)
>>> +{
>>> + char *home;
>>> + struct passwd pw_store, *pw_result;
>>> + char pw_buf[1024];
>>> +
>>> + /*FIXME: add check for issetugid as in libc of openbsd?*/
>>> + home = getenv("HOME");
>>> + if(home == NULL) {
>>> + getpwuid_r(getuid(), &pw_store, pw_buf, sizeof(pw_buf),
>>> &pw_result);
>>> + if(pw_result == NULL) {
>>> + return false;
>>> + }
>>> + home = pw_store.pw_dir;
>>> + }
>>> +
>>> + return glob_strlcpy(new_pat, home, new_pat_size) < new_pat_size
>>> + && glob_strlcat(new_pat, pat_after_tilde, new_pat_size) <
>>> new_pat_size;
>>> +}
>>> +
>>> +/* "~user/(...) case*/
>>> +static bool expand_tilde_named_user(const char *pat_after_tilde,
>>> char *new_pat, size_t new_pat_size)
>>> +{
>>> + struct passwd pw_store, *pw_result;
>>> + char pw_buf[1024], username[1024];
>>> + const char *slash_pos = strchr(pat_after_tilde, '/');
>>> + if(slash_pos == NULL) {
>>> + return false;
>>> + }
>>> +
>>> + ptrdiff_t pat_username_size = slash_pos - pat_after_tilde;
>>> + if(pat_username_size <= 0 || pat_username_size >=
>>> sizeof(username)) {
>>> + return false;
>>> + }
>>> + strncpy(username, pat_after_tilde, pat_username_size);
>>> + username[pat_username_size] = '\0';
>>> +
>>> + getpwnam_r(username, &pw_store, pw_buf, sizeof(pw_buf),
>>> &pw_result);
>>> + if (pw_result == NULL)
>>> + return false;
>>> +
>>> + return glob_strlcpy(new_pat, pw_store.pw_dir, new_pat_size) <
>>> new_pat_size
>>> + && glob_strlcat(new_pat, slash_pos, new_pat_size) <
>>> new_pat_size;
>>> +}
>>
>> It should be possible to reduce this code considerably, and to avoid
>> some of the large stack buffers. Certainly username[] does not need to
>> be 1k; I believe there's a macro for the max supported in limits.h or
>> such and it's something like 16 or 32 bytes.
>
> I searched and found there is a define for LOGIN_NAME_MAX which is 256.
> According to the useradd man page the maximum number of characters of
> an
> username is 32.
> There is also in limits.h _POSIX_NAME_MAX which is 14 and
> _POSIX_LOGIN_NAME_MAX
> which is 9.
> The openbsd pwd.h has a define for _PW_NAME_LEN which is 31 (without
> '\0').
>
> Which alternative is the best?
>
> (I updated the username arrays to be LOGIN_NAME_MAX)
>
> Relative to the pw_buf I don't think musl's code has any variable that
> defines
> the maximum supported passwd line.
> I used 1024 because that's what I saw in the openbsd code. They have a
> global
> char [] of size _PW_BUF_LEN which is 1024.
>
> Reduce the code in which way? Try to refactor some code of
> expand_tilde_cur_user and expand_tilde_named_user, of make the code
> "less
> verbose"?
>
>>
>>> int glob(const char *restrict pat, int flags, int (*errfunc)(const
>>> char *path, int err), glob_t *restrict g)
>>> {
>>> - const char *p=pat, *d;
>>> + const char *p, *d;
>>> + char new_pat[PATH_MAX + 1];
>>> struct match head = { .next = NULL }, *tail = &head;
>>> size_t cnt, i;
>>> size_t offs = (flags & GLOB_DOOFFS) ? g->gl_offs : 0;
>>> int error = 0;
>>> +
>>> + /*even if expanding fails(e.g. expansion make pat too big)
>>> + * we should try to match the ~ or ~user literally*/
>>> + bool should_expand_tilde = (flags & GLOB_TILDE) && (pat[0] ==
>>> '~');
>>> + if(should_expand_tilde && expand_tilde(pat, new_pat,
>>> sizeof(new_pat))) {
>>> + p = new_pat;
>>> + } else {
>>> + p = pat;
>>> + }
>>
>> Don't introduce gratuitous variables for expressions used just once.
>> The conditions going into that bool can just be part of the if.
> If I put it inside the if the line would be too big and those two
> checks go
> together.
> I think it's alot clearer.
>
>>
>> I haven't checked what happens when the input pat is too long to fit
>> in new_pat. This needs to be an error condition, not silent
>> truncation, but error conditions can't be handled until further down;
>> see commit 769f53598e781ffc89191520f3f8a93cb58db91f for why. Also,
>> new_pat should probably be a VLA whose length is 1 unless tilde
>> expansion is needed, and PATH_MAX otherwise, so that glob doesn't blow
>> an extra page on the stack when tilde expansion is not in use.
>
> I changed it to char new_pat[tilde_flag ? PATH_MAX : 1] where
> tilde_flag =
> flags & GLOB_TILDE.
> (new_pat no longer uses an extra page.)
>
> Alternatively I can put most of the code of glob in another function
> and only
> create a new pat buffer when we should try to expand the tilde.
> This would be good to reduce the scope of new_pat.
>
> When the expanded pattern is too big to fit new_pat, the expansion
> fails and
> the unexpanded pattern is used.
> According to glibc GLOB_TILDE should never make glob return an error.
> If
> anything related to the expansion of tilde fails it's supposed to
> continue glob
> with the pattern unexpanded.
> At least that how I interpret the manual:
> "
> If the username is invalid, or the home directory cannot be determined,
> then no
> substitution is performed.
> "
> What I'm trying to say is that GLOB_TILDE never produces an error, so
> the code
> that zeros the glob_t structure always runs.
>
> If the code that checks for leading '/' is put just before the call to
> match_dir, it'd be ok to put the code that calls expand_tilde after the
> glob_t
> zeroing and strlen checking and before the leading '/' code.
> The "downside" of this is that the caller of glob has a ton of leading
> '/' it
> could now fail the strlen check, but I think that's the fault of who is
> calling
> glob with "/////////////////////(...)"
> A patch for that is in attachment
> "move_leading_slash_check_down.patch".
>
>
>>
>> Rich
>
> I also send the patch with the things I fixed as attachment. It's
> still not okay.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: glob_tilde.patch --]
[-- Type: text/x-diff; name=glob_tilde.patch, Size: 3593 bytes --]
diff --git a/include/glob.h b/include/glob.h
index 76f6c1c6..477ddf2b 100644
--- a/include/glob.h
+++ b/include/glob.h
@@ -30,6 +30,7 @@ void globfree(glob_t *);
#define GLOB_APPEND 0x20
#define GLOB_NOESCAPE 0x40
#define GLOB_PERIOD 0x80
+#define GLOB_TILDE 0x1000
#define GLOB_NOSPACE 1
#define GLOB_ABORTED 2
diff --git a/src/regex/glob.c b/src/regex/glob.c
index 5b6ff124..16a0b1f7 100644
--- a/src/regex/glob.c
+++ b/src/regex/glob.c
@@ -8,6 +8,8 @@
#include <errno.h>
#include <stddef.h>
#include "libc.h"
+#include <pwd.h>
+#include <unistd.h>
struct match
{
@@ -154,13 +156,99 @@ static int sort(const void *a, const void *b)
return strcmp(*(const char **)a, *(const char **)b);
}
+static int concat(char *buf, size_t len, const char *s1, const char *s2)
+{
+ size_t n1 = strnlen(s1, len);
+ len -= n1;
+ size_t n2 = strnlen(s2, len);
+ len -= n2;
+ if (!len)
+ return 0;
+ memcpy(buf, s1, n1);
+ memcpy(buf+n1, s2, n2+1);
+ return 1;
+}
+
+/*"~" or "~/(...)" case*/
+static int expand_tilde_cur_user(const char *pat_after_tilde, char *new_pat, size_t new_pat_size)
+{
+ char *home;
+ struct passwd pw_store, *pw_result;
+ char pw_buf[LOGIN_NAME_MAX];
+
+ /*FIXME: add check for issetugid as in libc of openbsd?*/
+ home = getenv("HOME");
+ if(home == NULL) {
+ getpwuid_r(getuid(), &pw_store, pw_buf, sizeof(pw_buf), &pw_result);
+ if(pw_result == NULL) {
+ return 0;
+ }
+ home = pw_store.pw_dir;
+ }
+
+ return concat(new_pat, new_pat_size, home, pat_after_tilde);
+}
+
+/* "~user/(...) case*/
+static int expand_tilde_named_user(const char *pat_after_tilde, char *new_pat, size_t new_pat_size)
+{
+ struct passwd pw_store, *pw_result;
+ char pw_buf[1024], username[LOGIN_NAME_MAX];
+ const char *slash_pos = strchr(pat_after_tilde, '/');
+ if(slash_pos == NULL) {
+ return 0;
+ }
+
+ ptrdiff_t pat_username_size = slash_pos - pat_after_tilde;
+ if(pat_username_size <= 0 || pat_username_size >= sizeof(username)) {
+ return 0;
+ }
+ strncpy(username, pat_after_tilde, pat_username_size);
+ username[pat_username_size] = '\0';
+
+ getpwnam_r(username, &pw_store, pw_buf, sizeof(pw_buf), &pw_result);
+ if (pw_result == NULL)
+ return 0;
+
+ return concat(new_pat, new_pat_size, pw_store.pw_dir, slash_pos);
+}
+
+/* expands:
+ * ~ into /home/user/
+ * ~/asd into /home/user/asd
+ * ~user1/asd into /home/user1/asd
+ * the values for the home directory are taken from passwd
+ *
+ * returning true means successful expansion and that expanded_pat is valid
+ */
+static int expand_tilde(const char *pat, char *new_pat, int new_pat_size)
+{
+ const char *pat_after_tilde = pat + 1;
+ if(*pat_after_tilde == '\0' || *pat_after_tilde == '/') {
+ return expand_tilde_cur_user(pat_after_tilde, new_pat, new_pat_size);
+ } else {
+ return expand_tilde_named_user(pat_after_tilde, new_pat, new_pat_size);
+ }
+}
+
int glob(const char *restrict pat, int flags, int (*errfunc)(const char *path, int err), glob_t *restrict g)
{
- const char *p=pat, *d;
+ const char *p, *d;
+ const int tilde_flag = flags & GLOB_TILDE;
+ char new_pat[tilde_flag ? PATH_MAX : 1];
struct match head = { .next = NULL }, *tail = &head;
size_t cnt, i;
size_t offs = (flags & GLOB_DOOFFS) ? g->gl_offs : 0;
int error = 0;
+
+ /*even if expanding fails(e.g. expansion make pat too big)
+ * we should try to match the ~ or ~user literally*/
+ int should_expand_tilde = tilde_flag && (pat[0] == '~');
+ if(should_expand_tilde && expand_tilde(pat, new_pat, sizeof(new_pat))) {
+ p = new_pat;
+ } else {
+ p = pat;
+ }
if (*p == '/') {
for (; *p == '/'; p++);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Implementation of GLOB_TILDE
@ 2017-07-28 16:36 Nelo-Thara Wallus
0 siblings, 0 replies; 6+ messages in thread
From: Nelo-Thara Wallus @ 2017-07-28 16:36 UTC (permalink / raw)
To: musl
Morning list,
In January Bernardo Pascoal Figueiredo sent in a patch to add support
for GLOB_TILDE[1], which hasn't been merged yet.
We've hit that missing expansion with compiling i3 on musl systems[2].
Since GLOB_TILDE has been on the roadmap for 1.1.16[3] I wanted to ask
if it'd be possible to merge the patch for 1.1.17.
The patch applies to current master with fuzz factor 3.
Thanks and Cheers,
Nelo
[1]: http://www.openwall.com/lists/musl/2017/01/17/1
[2]: https://bugs.gentoo.org/show_bug.cgi?id=609306
[3]: http://wiki.musl-libc.org/wiki/Roadmap
--
/"\ ASCII Ribbon Campaign
\ / - against HTML emails
X - against proprietory attachments
/ \ http://en.wikipedia.org/wiki/ASCII_Ribbon_Campaign
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-07-28 16:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-17 15:44 Implementation of GLOB_TILDE Bernardo Pascoal Figueiredo
2017-01-17 15:49 ` Bernardo Pascoal Figueiredo
2017-01-17 19:39 ` Rich Felker
2017-01-18 19:51 ` Bernardo Pascoal Figueiredo
2017-01-20 9:56 ` Bernardo Pascoal Figueiredo
2017-07-28 16:36 Nelo-Thara Wallus
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).