bash-shipped getcwd() replacement does not work on interix.

[Michael Haubenwallner]

Machine: i586
OS: interix5.2
Compiler: gcc
Compilation CFLAGS: -DPROGRAM='bash' -DCONF_HOSTTYPE='i586'
-DCONF_OSTYPE='interix5.2' -DCONF_MACHTYPE='i586-pc-interix5.2'
-DCONF_VENDOR='pc'
-DLOCALEDIR='/tools/snapshot/prefix-launcher-1pre.20071219/i586-pc-interix5.2/share/locale' -DPACKAGE='bash' -DLOCAL_PREFIX=/tools/snapshot/prefix-launcher-1pre.20071219/i586-pc-interix5.2 -DSHELL -DHAVE_CONFIG_H -DNO_MAIN_ENV_ARG -DBROKEN_DIRENT_D_INO -D_POSIX_SOURCE -I. -I/tss/prefix-launcher-1pre.20071219/buildroot/bash/bash-3.2 -I/tss/prefix-launcher-1pre.20071219/buildroot/bash/bash-3.2/include -I/tss/prefix-launcher-1pre.20071219/buildroot/bash/bash-3.2/lib -g -O2
uname output: Interix pc312001 5.2 SP-9.0.3790.3034 x86
Intel_x86_Family6_Model15_Stepping6
Machine Type: i586-pc-interix5.2

Bash Version: 3.2
Patch Level: 33
Release Status: release

Description:
Bash uses getcwd-replacement if libc provides getcwd without the
feature of allocating the buffer when called without one.
This override is done in config-bot.h, with an exception for
solaris already.
Problem now is that getcwd-replacement does not work on Interix
(SUA 5.2 here).
Now there's only one source location in builtins/common.c really
relying on getcwd(0,0) allocating the buffer.
But even here is some conditional code on GETCWD_BROKEN.
So why not simply don't require allocation-feature of getcwd at all
and use getcwd-replacement only if libc does not provide one ?

Repeat-By:
$ PWD= /tools/snapshot/prefix-launcher-1pre.20071219/i586-pc-interix5.2/bin/bash
shell-init: error retrieving current directory: getcwd: cannot access parent directories: No such file or directory

Fix: (patch attached)
builtins/common.c:
Do not depend on getcwd() doing buffer allocation.
config-bot.h:
Ignore GETCWD_BROKEN, keep HAVE_GETCWD as is.
Additionally, the check for GETCWD_BROKEN can be dropped
from configure.in and aclocal.m4.

Thanks!

/haubi/
--
Michael Haubenwallner
Gentoo on a different level

[Andreas Schwab]

Michael Haubenwallner <ha...@gentoo.org> writes:

> diff -ru builtins/common.c builtins/common.c
> --- builtins/common.c Wed Dec 19 10:30:07 2007
> +++ builtins/common.c Wed Dec 19 10:34:58 2007
> @@ -479,11 +479,8 @@
>
> if (the_current_working_directory == 0)
> {
> -#if defined (GETCWD_BROKEN)
> - the_current_working_directory = getcwd (0, PATH_MAX);
> -#else
> - the_current_working_directory = getcwd (0, 0);
> -#endif
> + char *t = xmalloc(PATH_MAX);
> + the_current_working_directory = getcwd (t, PATH_MAX);

The length of the cwd may be bigger than PATH_MAX.

Andreas.

--
Andreas Schwab, SuSE Labs, sch...@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."

[Chet Ramey]

Michael Haubenwallner wrote:
> Machine: i586
> OS: interix5.2
> Compiler: gcc
> Compilation CFLAGS: -DPROGRAM='bash' -DCONF_HOSTTYPE='i586'
> -DCONF_OSTYPE='interix5.2' -DCONF_MACHTYPE='i586-pc-interix5.2'
> -DCONF_VENDOR='pc'
> -DLOCALEDIR='/tools/snapshot/prefix-launcher-1pre.20071219/i586-pc-interix5.2/share/locale' -DPACKAGE='bash' -DLOCAL_PREFIX=/tools/snapshot/prefix-launcher-1pre.20071219/i586-pc-interix5.2 -DSHELL -DHAVE_CONFIG_H -DNO_MAIN_ENV_ARG -DBROKEN_DIRENT_D_INO -D_POSIX_SOURCE -I. -I/tss/prefix-launcher-1pre.20071219/buildroot/bash/bash-3.2 -I/tss/prefix-launcher-1pre.20071219/buildroot/bash/bash-3.2/include -I/tss/prefix-launcher-1pre.20071219/buildroot/bash/bash-3.2/lib -g -O2
> uname output: Interix pc312001 5.2 SP-9.0.3790.3034 x86
> Intel_x86_Family6_Model15_Stepping6
> Machine Type: i586-pc-interix5.2
>
> Bash Version: 3.2
> Patch Level: 33
> Release Status: release
>
> Description:
> Bash uses getcwd-replacement if libc provides getcwd without the
> feature of allocating the buffer when called without one.
> This override is done in config-bot.h, with an exception for
> solaris already.
> Problem now is that getcwd-replacement does not work on Interix
> (SUA 5.2 here).

I'd be more interested in knowing why it doesn't work in this case,
instead of discarding it. Since I neither have nor use Interix, I
need someone who does to investigate the issue a little bit.

Chet

--
``The lyf so short, the craft so long to lerne.'' - Chaucer
Live Strong. No day but today.
Chet Ramey, ITS, CWRU ch...@case.edu http://cnswww.cns.cwru.edu/~chet/

[Eric Blake]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

According to Andreas Schwab on 12/20/2007 4:30 AM:

>> + char *t = xmalloc(PATH_MAX);
>> + the_current_working_directory = getcwd (t, PATH_MAX);
>
> The length of the cwd may be bigger than PATH_MAX.

On most systems, yes. Interix, however, probably follows suit with cygwin
1.5.x, since most Windows syscalls enforce that relative path names can't
generate paths whose corresponding absolute path is longer than a puny 256
PATH_MAX (the cygwin developers are working towards avoiding this
limitation in cygwin 1.7.0 by using lower-level NT syscalls that allow up
to 32k in absolute length, and will be raising their PATH_MAX to at least
the POSIX XSI minimum of 1k, but aren't there yet). But I agree that the
patch as posted is wrong for non-Interix platforms.

It may also be worth inspecting the gnulib replacement for getcwd, to see
how it handles the problem:
http://git.sv.gnu.org/gitweb/?p=gnulib.git;a=blob;f=lib/getcwd.c

- --
Don't work too hard, make some time for fun as well!

Eric Blake eb...@byu.net
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFHamw184KuGfSFAYARArOXAJkBwR6CbME0zyACCFbbe2g8XLmOKACgzdoJ
Y0p53TTfWOrxjctD8Bkck2Q=
=0ssG
-----END PGP SIGNATURE-----

[Michael Haubenwallner]

On Thu, 2007-12-20 at 12:30 +0100, Andreas Schwab wrote:
> Michael Haubenwallner <ha...@gentoo.org> writes:
>
> > diff -ru builtins/common.c builtins/common.c
> > --- builtins/common.c Wed Dec 19 10:30:07 2007
> > +++ builtins/common.c Wed Dec 19 10:34:58 2007
> > @@ -479,11 +479,8 @@
> >
> > if (the_current_working_directory == 0)
> > {
> > -#if defined (GETCWD_BROKEN)
> > - the_current_working_directory = getcwd (0, PATH_MAX);
> > -#else
> > - the_current_working_directory = getcwd (0, 0);
> > -#endif

> > + char *t = xmalloc(PATH_MAX);
> > + the_current_working_directory = getcwd (t, PATH_MAX);
>
> The length of the cwd may be bigger than PATH_MAX.

Eventually - but there are three (ok, two) other locations in bash-3.2
where buffer[PATH_MAX] is passed to getcwd():

1) jobs.c:
current_working_directory()
2) parse.y:
decode_prompt_string()

3) lib/readline/examples/fileman.c:
com_pwd()
ok, this just is an example, and uses 1024 instead of PATH_MAX.

Instead of using PATH_MAX, why not have some xgetcwd() instead, doing
malloc (when getcwd does not allocate itself), and increase the buffer
when getcwd() returns ERANGE ?

[Michael Haubenwallner]

It is because readdir() returns 0 (zero) for (struct dirent).(d_ino),
while stat() returns useful values for (struct stat).(st_ino), so their
equal-comparison never succeeds.

Now, while trying to get inode number from stat() rather than readdir(),
I've seen another bug unrelated to readdir()/stat(), but still in
getcwd() replacement, causing a coredump here.

It is with the memcpy() from the internal buffer to the allocated return
buffer, but only when there is a minimal buffer size specified - wth. is
this done in get_working_directory() when GETCWD_BROKEN is defined:
Does Solaris (config-bot.h) allocate the buffer when a size is passed ?

Attached patch fixes this one issue, by still allocating at least
provided buffer size, but doing the memcpy with real path length.
When done with buffer size, memcpy reads beyond the end of the source
buffer on the stack. The SIGSEGV was caused here because it has read
beyond the whole stack frame page.

[Michael Haubenwallner]

Attached patch should fix this issue, not relying on readdir() returning
valid d_ino, but doing stat() always instead.

But eventually there should be a configure-check or sth. like that if
readdir returns valid d_ino, and subsequently avoid the additional stat.

Moving alloca() into separate function was necessary because there is no
realloca() or sth. like that, and wasting stack for each iteration is
bad.

[Chet Ramey]

Michael Haubenwallner wrote:
>> It is because readdir() returns 0 (zero) for (struct dirent).(d_ino),
>> while stat() returns useful values for (struct stat).(st_ino), so their
>> equal-comparison never succeeds.
>
> Attached patch should fix this issue, not relying on readdir() returning
> valid d_ino, but doing stat() always instead.

You didn't attach one.

[Michael Haubenwallner]

On Sat, 2007-12-22 at 10:13 -0500, Chet Ramey wrote:
> Michael Haubenwallner wrote:
> >> It is because readdir() returns 0 (zero) for (struct dirent).(d_ino),
> >> while stat() returns useful values for (struct stat).(st_ino), so their
> >> equal-comparison never succeeds.
> >
> > Attached patch should fix this issue, not relying on readdir() returning
> > valid d_ino, but doing stat() always instead.
>
> You didn't attach one.

Uh oh, indeed, sorry. Here it is.