From: Rex D. <rd...@ma...> - 2005-08-30 13:30:12
Attachments:
sbcl-0.9.4-ADDR_NO_RANDOMIZE-2.patch
|
> From: Juho Snellman <jsnell@ik...> > On Mon, Aug 29, 2005 at 03:29:15PM +0200, Florian Weimer wrote: > > * Juho Snellman: > > > > > This has the downside of not helping (though not harming, either) if > > > /proc is not mounted. I don't think there's any other reliable method > > > for finding out the path of the current executable. > > > > I think this causes Lisp processes to show up as "exe" in tools like > > ps and top. > > Damn. I'd only thought of checking ps (which shows the correct name on > my systems), not top. This shouldn't be a problem, since > /proc/self/exe is implemented as a symlink. Just follow the link > first, then execute. Here's an updated version of the original patch using readlink. I've verified it works happily on fc4, in conjunction with sbcl's submission to Fedora Extras: http://bugzilla.redhat.com/bugzilla/166800 -- Rex |
From: Christophe R. <cs...@ca...> - 2005-08-30 14:15:33
|
Rex Dieter <rd...@ma...> writes: > + char *buf; > + size_t bufsiz = sizeof(buf)*MAXPATHLEN; > + buf = (char *)malloc(bufsiz ); Shouldn't we be checking the return value of malloc() here? Cheers, Christophe |
From: Rex D. <rd...@ma...> - 2005-08-30 14:16:41
|
Christophe Rhodes wrote: >Rex Dieter <rd...@ma...> writes: > > > >>+ char *buf; >>+ size_t bufsiz = sizeof(buf)*MAXPATHLEN; >>+ buf = (char *)malloc(bufsiz ); >> >> > >Shouldn't we be checking the return value of malloc() here? > > Yeah, I knew someone would see that. (-: -- Rex |
From: Rex D. <rd...@ma...> - 2005-08-30 14:46:48
Attachments:
sbcl-0.9.4-ADDR_NO_RANDOMIZE.patch
|
Christophe Rhodes wrote: >Rex Dieter <rd...@ma...> writes: > > > >>+ char *buf; >>+ size_t bufsiz = sizeof(buf)*MAXPATHLEN; >>+ buf = (char *)malloc(bufsiz ); >> >> > >Shouldn't we be checking the return value of malloc() here? > > > How about this? -- Rex |
From: Todd S. <ts...@op...> - 2005-08-30 15:13:01
|
Rex Dieter <rd...@ma...> writes: > How about this? Well, from the "No good deed goes unpunished" department... > [...] > +#if defined(LISP_FEATURE_X86) > + if ((major_version > 2) || (minor_version >= 6)) { Shouldn't that be "&&" instead of "||"? > + long pers = personality(-1); > + /* 0x40000 aka. ADDR_NO_RANDOMIZE */ > + if (!(pers & 0x40000)) { > + if (personality(pers | 0x40000) != -1) { > + /* Use /proc/self/exe instead of trying to figure out the > + * executable path from PATH and argv[0], since that's reliable. > + */ > + char *buf; > + size_t bufsiz = sizeof(buf)*MAXPATHLEN; > + buf = (char *)malloc(bufsiz ); > + if (buf) > + if (readlink("/proc/self/exe", *buf, bufsiz)) > + execve(buf, argv, envp); I think this is still broken. It should be "buf" instead of "*buf" in the readlink call. Also, readlink returns -1 on failure, which would be interpreted as success, and finally readlink doesn't null terminate the buffer. I think you want something like this (untested): char buf[MAXPATHLEN+1]; int rc = readlink ("/proc/self/exe", buf, sizeof (buf) -1); if (rc != -1) { buf[rc] = 0; execve (buf, argv, envp); } > + } > + /* Either changing the personality or execve() failed. Either > + * way we might as well continue, and hope that the random > + * memory maps are ok this time around. > + */ > + fprintf(stderr, "WARNING: Couldn't re-execute SBCL with the proper personality flags (maybe /proc isn't mounted?). Trying to continue anyway.\n"); > + } > + } > +#endif > } -- Todd Sabin <ts...@op...> |
From: Thiemo S. <th...@ne...> - 2005-08-30 15:32:09
|
Todd Sabin wrote: > Rex Dieter <rd...@ma...> writes: > > > How about this? > > Well, from the "No good deed goes unpunished" department... > > > [...] > > +#if defined(LISP_FEATURE_X86) > > + if ((major_version > 2) || (minor_version >= 6)) { > > Shouldn't that be "&&" instead of "||"? Better even: if ((major_version == 2) && (minor_version >= 6) || (major_version > 3)) { > > + long pers = personality(-1); > > + /* 0x40000 aka. ADDR_NO_RANDOMIZE */ > > + if (!(pers & 0x40000)) { > > + if (personality(pers | 0x40000) != -1) { > > + /* Use /proc/self/exe instead of trying to figure out the > > + * executable path from PATH and argv[0], since that's reliable. > > + */ > > + char *buf; > > + size_t bufsiz = sizeof(buf)*MAXPATHLEN; > > + buf = (char *)malloc(bufsiz ); > > + if (buf) > > + if (readlink("/proc/self/exe", *buf, bufsiz)) > > + execve(buf, argv, envp); > > I think this is still broken. It should be "buf" instead of "*buf" in > the readlink call. Also, readlink returns -1 on failure, which would > be interpreted as success, and finally readlink doesn't null terminate > the buffer. I think you want something like this (untested): > > char buf[MAXPATHLEN+1]; > int rc = readlink ("/proc/self/exe", buf, sizeof (buf) -1); > if (rc != -1) { > buf[rc] = 0; > execve (buf, argv, envp); > } PATH_MAX+1 instead of MAXPATHLEN+1 because it's standardized, and probably simply PATH_MAX instead of sizeof (buf) -1, otherwise agreed. Thiemo |
From: Todd S. <ts...@op...> - 2005-08-30 15:46:54
|
Thiemo Seufer <th...@ne...> writes: > Todd Sabin wrote: >> I think this is still broken. It should be "buf" instead of "*buf" in >> the readlink call. Also, readlink returns -1 on failure, which would >> be interpreted as success, and finally readlink doesn't null terminate >> the buffer. I think you want something like this (untested): >> >> char buf[MAXPATHLEN+1]; >> int rc = readlink ("/proc/self/exe", buf, sizeof (buf) -1); >> if (rc != -1) { >> buf[rc] = 0; >> execve (buf, argv, envp); >> } > > PATH_MAX+1 instead of MAXPATHLEN+1 because it's standardized, and > probably simply PATH_MAX instead of sizeof (buf) -1, otherwise agreed. Yes, PATH_MAX is probably better. I generally use sizeof (XXX) when possible, because then if you want to change the size or the define (from say MAXPATHLEN to PATH_MAX :)), you only have to do it in one place. Also, the -1 is necessary to avoid the corner case of readlink completely filling the buffer, leaving you with the options of not null terminating, truncating the last character, or going outside the array bounds. -- Todd Sabin <ts...@op...> |
From: Rex D. <rdi...@un...> - 2005-08-30 15:53:26
|
Todd Sabin wrote: > Also, the -1 is necessary to > avoid the corner case of readlink completely filling the buffer, > leaving you with the options of not null terminating, truncating the > last character, or going outside the array bounds. Nowhere in my execve manpage does it say the (char *) containing the path needs to be null-terminated. -- Rex p.s. I'm on sbcl-devel, no need to Cc me. |
From: Thiemo S. <th...@ne...> - 2005-08-30 16:15:25
|
Rex Dieter wrote: > Todd Sabin wrote: > > Also, the -1 is necessary to > >avoid the corner case of readlink completely filling the buffer, > >leaving you with the options of not null terminating, truncating the > >last character, or going outside the array bounds. > > Nowhere in my execve manpage does it say the (char *) containing the > path needs to be null-terminated. How else could execve find the end of the pathname? Of course it has to be a valid C string. Thiemo |
From: Rex D. <rd...@ma...> - 2005-08-30 16:32:46
|
Rex Dieter wrote: > Todd Sabin wrote: > >> Also, the -1 is necessary to >> avoid the corner case of readlink completely filling the buffer, >> leaving you with the options of not null terminating, truncating the >> last character, or going outside the array bounds. > > > Nowhere in my execve manpage does it say the (char *) containing the > path needs to be null-terminated. Upon further reflection, it had better be terminated. Ignore me. (-: -- Rex |
From: Thiemo S. <th...@ne...> - 2005-08-30 16:11:28
|
Todd Sabin wrote: > Thiemo Seufer <th...@ne...> writes: > > > Todd Sabin wrote: > >> I think this is still broken. It should be "buf" instead of "*buf" in > >> the readlink call. Also, readlink returns -1 on failure, which would > >> be interpreted as success, and finally readlink doesn't null terminate > >> the buffer. I think you want something like this (untested): > >> > >> char buf[MAXPATHLEN+1]; > >> int rc = readlink ("/proc/self/exe", buf, sizeof (buf) -1); > >> if (rc != -1) { > >> buf[rc] = 0; > >> execve (buf, argv, envp); > >> } > > > > PATH_MAX+1 instead of MAXPATHLEN+1 because it's standardized, and > > probably simply PATH_MAX instead of sizeof (buf) -1, otherwise agreed. > > Yes, PATH_MAX is probably better. > > I generally use sizeof (XXX) when possible, because then if you want > to change the size or the define (from say MAXPATHLEN to PATH_MAX :)), > you only have to do it in one place. For nonstandard defines that's preferable, but for C standard macros it's code obfuscation. We actually want readlink() to read the buffer up to the system limit, a sizeof() expression doesn't document that well. > Also, the -1 is necessary to > avoid the corner case of readlink completely filling the buffer, > leaving you with the options of not null terminating, truncating the > last character, or going outside the array bounds. That's why I said PATH_MAX+1 for the buffer but PATH_MAX as size argument. :-) Thiemo |
From: Juho S. <js...@ik...> - 2005-08-30 16:53:08
|
<th...@ne...> wrote: > Todd Sabin wrote: >> Rex Dieter <rd...@ma...> writes: >> > +#if defined(LISP_FEATURE_X86) >> > + if ((major_version > 2) || (minor_version >= 6)) { >> >> Shouldn't that be "&&" instead of "||"? > > Better even: > > if ((major_version == 2) && (minor_version >= 6) > || (major_version > 3)) { [ s/> 3/> 2/ ] The original is actually correct, but doesn't make sense when taken out of context (major_version < 2 loses earlier in the function). But I admit it's probably better to be explicit about this instead of confusing the poor maintenance programmer. Other than PATH_MAX/PATH_MAX-1 vs. PATH_MAX+1/PATH_MAX the currently suggested version matches what I already had in my tree. I'll make that change too. -- Juho Snellman |
From: Thiemo S. <th...@ne...> - 2005-08-30 17:06:07
|
Juho Snellman wrote: > <th...@ne...> wrote: > > Todd Sabin wrote: > >> Rex Dieter <rd...@ma...> writes: > >> > +#if defined(LISP_FEATURE_X86) > >> > + if ((major_version > 2) || (minor_version >= 6)) { > >> > >> Shouldn't that be "&&" instead of "||"? > > > > Better even: > > > > if ((major_version == 2) && (minor_version >= 6) > > || (major_version > 3)) { > > [ s/> 3/> 2/ ] I meant [ s|> 3|>= 3| ] actually, as this preserves the literal kernel version number. Thiemo |
From: Thiemo S. <th...@ne...> - 2005-08-30 14:56:15
|
Christophe Rhodes wrote: > Rex Dieter <rd...@ma...> writes: > > > + char *buf; > > + size_t bufsiz = sizeof(buf)*MAXPATHLEN; > > + buf = (char *)malloc(bufsiz ); > > Shouldn't we be checking the return value of malloc() here? No, it should rather be char buf[PATH_MAX]; instead of leaking memory with a size of MAXPATHLEN * pointer size. Thiemo |
From: Todd S. <ts...@op...> - 2005-08-30 16:45:31
|
Thiemo Seufer <th...@ne...> writes: > Todd Sabin wrote: >> I generally use sizeof (XXX) when possible, because then if you want >> to change the size or the define (from say MAXPATHLEN to PATH_MAX :)), >> you only have to do it in one place. > > For nonstandard defines that's preferable, but for C standard macros > it's code obfuscation. We actually want readlink() to read the buffer > up to the system limit, a sizeof() expression doesn't document that well. A matter of taste, I guess. I'm probably somewhat biased in that I've done a fair amount of code auditing in the past, and anytime I see someone use a define like that, I feel like I have to check that it's the same one they used to declare the buffer, whereas it's less necessary if sizeof is used. >> Also, the -1 is necessary to >> avoid the corner case of readlink completely filling the buffer, >> leaving you with the options of not null terminating, truncating the >> last character, or going outside the array bounds. > > That's why I said PATH_MAX+1 for the buffer but PATH_MAX as size argument. :-) Oh, right, sorry. -- Todd Sabin <ts...@op...> |