[ruby-core:40840] A quiz: Re: Second patch attempt for PATH_MAX fix in ruby1.9.1- 1.9.3~rc1-3

[ruby-core:40840] A quiz: Re: Second patch attempt for PATH_MAX fix in ruby1.9.1- 1.9.3~rc1-3

[Svante Signell]

Hello,

I'm working on porting packages having PATH_MAX issues and ruby1.9.1 is
one of them. PATH_MAX is not defined for GNU/Hurd. The code analysis
below shows that it would be possible to use dynamic allocation of the
buffer binary_filename using malloc and free.

However, Samuel Thibault, a large contributor to GNU/Hurd and a Debiab
Developer, advised me to ask here is it is possible or if a
fixed-length buffer should be used? Especially if it is safe to call
rb_dump_backtrace_with_lines from a *Unix* signal handler.

So if dynamic allocation is not possible, can you in the next release
provide a conditional definition of PATH_MAX?

#ifndef PATH_MAX
#define PATH_MAX 4096
#endif

> > Below is a patch attempt to solve the PATH_MAX issue on the latest
> > ruby1.9.1.

Code analysis follows below:

binary_filename is a global variable:
/* Avoid consuming stack as this module may be used from signal handler
*/
static char *binary_filename = NULL;

Call order:
rb_dump_backtrace_with_lines()->fill_lines()->
follow_debuglink()->parse_debug_line()->parse_debug_line_cu()

rb_dump_backtrace_with_lines() copies binary_filename content to path.
follow_debuglink() creates new content of binary_filename.

binary_filename needed in parse_debug_line() and parse_debug_line_cu()

I think the following might/might not work:

rb_dump_backtrace_with_lines():
===============================

creates content in path:
        if (!get_path_from_symbol(syms[i], &path, &len)) {
            continue;
        }

allocates a new value to binary_filename:
       binary_filename = (char *)(malloc(len) + 1);

A new pointer for binary_filename is created.

        strncpy(binary_filename, path, len);
	binary_filename[len] = '\0';

calls fill_lines():
        fill_lines(num_traces, trace, syms, 1, &lines[i], lines);


fill_lines():
=============
opens a file:
   fd = open(binary_filename, O_RDONLY);

makes a comparison:
         if (get_path_from_symbol(syms[i], &path, &len) &&
                !strncmp(path, binary_filename, len)) {
            lines[i].line = -1;
        }

calls follow_debuglink():
        if (gnu_debuglink_shdr && check_debuglink) {
            follow_debuglink(file + gnu_debuglink_shdr->sh_offset,
                             num_traces, traces, syms,
                             current_line, lines);

calls parse_debug_line():
   parse_debug_line(num_traces, traces,
                     file + debug_line_shdr->sh_offset,
                     debug_line_shdr->sh_size,
                     lines);

follow_debuglink()
==================
static const char global_debug_dir[] = "/usr/lib/debug";

Here the previously malloced binary_filename is needed!

    subdir = (char *)alloca(strlen(binary_filename) + 1);
    strcpy(subdir, binary_filename);

Here the content of binary_filename is copied to subdir. 
Binary_filename can be disposed.

    free(binary_filename)

free(binary_filename); is needed here in order not to allocate
a new pointer without disposing the old one??

The binary_filename is concatenated to:
global_debug_dir+subdir+debuglink

    len = strlen(global_debug_dir) + strlen(subdir) + strlen(debuglink);
    binary_filename = (char*)(malloc(len) + 1);

A new pointer for binary_filename is created

    strcpy(binary_filename, global_debug_dir);
    strncat(binary_filename, subdir, strlen(subdir));
    strncat(binary_filename, debuglink, strlen(debuglink));
    binary_filename[len] = '\0';

}

parse_debug_line():
===================
Here the previously malloced binary_filename is needed!

    while (debug_line < debug_line_end) {
        parse_debug_line_cu(num_traces, traces, &debug_line, lines);

prints using binary_filename

parse_debug_line_cu():
======================
Here the previously malloced binary_filename is needed!

prints using binary_filename

Thanks!

[ruby-core:40846] Re: A quiz: Re: Second patch attempt for PATH_MAX fix in ruby1.9.1- 1.9.3~rc1-3

[NARUSE, Yui]

2011/11/8 Svante Signell <svante.signell@telia.com>:
> I'm working on porting packages having PATH_MAX issues and ruby1.9.1 is
> one of them. PATH_MAX is not defined for GNU/Hurd. The code analysis
> below shows that it would be possible to use dynamic allocation of the
> buffer binary_filename using malloc and free.
>
> However, Samuel Thibault, a large contributor to GNU/Hurd and a Debiab
> Developer, advised me to ask here is it is possible or if a
> fixed-length buffer should be used? Especially if it is safe to call
> rb_dump_backtrace_with_lines from a *Unix* signal handler.
>
> So if dynamic allocation is not possible, can you in the next release
> provide a conditional definition of PATH_MAX?
>
> #ifndef PATH_MAX
> #define PATH_MAX 4096
> #endif

I committed in r33675.

>> > Below is a patch attempt to solve the PATH_MAX issue on the latest
>> > ruby1.9.1.

This means trunk branch of ruby repo?

> Code analysis follows below:

You may already noticed, following codes are run when the program is
crashed like segv.
On such situation, it hardly use malloc.

-- 
NARUSE, Yui  <naruse@airemix.jp>

[ruby-core:40847] Re: A quiz: Re: Second patch attempt for PATH_MAX fix in ruby1.9.1- 1.9.3~rc1-3

[Svante Signell]

On Wed, 2011-11-09 at 00:36 +0900, NARUSE, Yui wrote:
> 2011/11/8 Svante Signell <svante.signell@telia.com>:
> > I'm working on porting packages having PATH_MAX issues and ruby1.9.1 is
> > one of them. PATH_MAX is not defined for GNU/Hurd. The code analysis
> > below shows that it would be possible to use dynamic allocation of the
> > buffer binary_filename using malloc and free.
> >
> > However, Samuel Thibault, a large contributor to GNU/Hurd and a Debiab
> > Developer, advised me to ask here is it is possible or if a
> > fixed-length buffer should be used? Especially if it is safe to call
> > rb_dump_backtrace_with_lines from a *Unix* signal handler.
> >
> > So if dynamic allocation is not possible, can you in the next release
> > provide a conditional definition of PATH_MAX?
> >
> > #ifndef PATH_MAX
> > #define PATH_MAX 4096
> > #endif
> 
> I committed in r33675.

Thanks!

> >> > Below is a patch attempt to solve the PATH_MAX issue on the latest
> >> > ruby1.9.1.
> 
> This means trunk branch of ruby repo?

The code I compiled is the latest one in Debian experimental a while
ago: ruby1.9.1-1.9.3~rc1-1. Now the latest version in unstable seems to
be: 1.9.3.0-1

> > Code analysis follows below:
> 
> You may already noticed, following codes are run when the program is
> crashed like segv.
> On such situation, it hardly use malloc.

OK, so a fixed-length buffer is the only solution. Thanks!

[ruby-core:40851] Re: A quiz: Re: Second patch attempt for PATH_MAX fix in ruby1.9.1- 1.9.3~rc1-3

[KOSAKI Motohiro]

2011/11/8 Svante Signell <svante.signell@telia.com>:
> Hello,
>
> I'm working on porting packages having PATH_MAX issues and ruby1.9.1 is
> one of them. PATH_MAX is not defined for GNU/Hurd. The code analysis
> below shows that it would be possible to use dynamic allocation of the
> buffer binary_filename using malloc and free.
>
> However, Samuel Thibault, a large contributor to GNU/Hurd and a Debiab
> Developer, advised me to ask here is it is possible or if a
> fixed-length buffer should be used? Especially if it is safe to call
> rb_dump_backtrace_with_lines from a *Unix* signal handler.
>
> So if dynamic allocation is not possible, can you in the next release
> provide a conditional definition of PATH_MAX?

At first, thank you very much for your contribution. Your mail has
both a detailed
analysis and a patch, and then we can apply it fearlessly.

We don't refuse cooperation with platform developers as far as they respect
our code maintenance and keeping code cleanness.

Cheers.