[PATCH] realpath stack usage 8k -> 4k

[Denys Vlasenko]

On Wednesday 07 May 2008 19:11, Carmelo AMOROSO wrote:
> >> Proposed patch uses user-supplied buffer directly,
> >> without intermediate on-stack copy.
> >> This can only make a difference if user supplied
> >> a buffer which is too small - thus user breaks API.
> >>
> >> Failure scenario:
> >>
> >> realpath("/link_name", user_buffer)
> >>
> >> /link_name -> /very_long_name_which_fits_into_PATH_MAX_and_is_also_a_link
> >> -> -> /shorter_name
> >>
> >> If user will give e.g. 40-char user_buffer, current implementation
> >> will work, patched one will overflow user_buffer by intermediate name.
> >>
> >> This should not be a problem - user must supply PATH_MAX sized buffer,
> >> and in this case patched version also works correctly.
> >>     
> >
> > And the following patch on top of previous one reuses copy_buf[]
> > for readlink, eliminating link_buf[]. In order to make it work,
> > "source" pathname is kept at the end of copy_buf, not at the
> > beginning (so that last NUL byte is the last byte of the copy_buf[]).
> >
> > The situation when readlink returns link name which is too long
> > (so that it overwrites pathname), was resulting in ENAMETOOLONG
> > error return. This patch does the same - the fact the we now trash
> > pathname does not matter, as we are not returning it to the user.
> >
> > Run tested.
> >
> > Can somebody review these patches please?
> > --
> > vda
> >   
> 
> Hi Denys,
> I'm just now playing with uclibc-trunk (linuxthreads.old implementation) 
> and discovered
> that your latest change on realpath implementation brakes test-canon 
> test from uclibc test-suite.
> Indeed, I've run exactly the same on nptl branch (that contains the 
> previous implementation of
> the realpath function) and it works fine.

Just built and run test here. Result:

# ./test-canon
./test-canon: flunked test 3 (expected `/etc', got `/.local/etc')
./test-canon: flunked test 4 (expected `/etc', got `/.local/etc')
./test-canon: flunked test 7 (expected resolved `/etc/doesNotExist', got `/.local/etc/doesNotExist')
./test-canon: flunked test 14 (expected resolved `', got `/.1/usr/srcdevel/uclibc/fix/uClibc.t7/test/stdlib/SYMLINK_LOOP')
./test-canon: flunked test 15 (expected resolved `', got `/.1/usr/srcdevel/uclibc/fix/uClibc.t7/test/stdlib/SYMLINK_LOOP')
./test-canon: flunked test 18 (expected `/etc', got `/.local/etc')
./test-canon: flunked test 20 (expected `/etc', got `/.local/etc')
./test-canon: flunked test 22 (expected `/etc', got `/.local/etc')
./test-canon: flunked test 24 (expected `/etc', got `/.local/etc')

Except 14 and 15, test makes an assumption that /etc is not a symlink.
On my machine it is not true, so it fails.

Tests 14 and 15 say in source:

  /* 10 */
  {"./doesExist/../doesExist",          "./doesExist"},
  {"foobar",                            0, "./foobar", ENOENT},
  {".",                                 "."},
  {"./foobar",                          0, "./foobar", ENOENT},
#ifdef __UCLIBC__
  /* we differ from glibc here, but POSIX allows it as it says that if we did
   * not successfuly complete, the value of resolved_path is undefined */
  {"SYMLINK_LOOP",                      0, "", ELOOP},
  /* 15 */
  {"./SYMLINK_LOOP",                    0, "", ELOOP},
#else

I can make it work so that test passes. However, as this comment says,
POSIX says nothing about return buffer's contents, so alternative
fix would be to not check its contents, only return value and errno.

Do you want me to make readlink return "" or to stop testing
buffer's contents? Your pick.
--
vda