A quiz: Re: Second patch attempt for PATH_MAX fix in ruby1.9.1- 1.9.3~rc1-3

[Svante Signell <svante.signell@telia.com>]

On Fri, 2011-10-14 at 11:17 +0200, Pino Toscano wrote:
> Hi,
> 
> Alle giovedì 13 ottobre 2011, Svante Signell ha scritto:
> > Below is a patch attempt to solve the PATH_MAX issue on the latest
> > ruby1.9.1.
> 
> I don't think this is correct: various functions in addr2line.c seem to 
> assume the buffer is always the same, and they call themselves 
> recursively (e.g. rb_dump_backtrace_with_lines (sets binary_filename) -> 
> fill_lines -> follow_debuglink (sets binary_filename) -> fill_lines -> 
> ...). With your patch, it would also cause the binary_filename pointer 
> to be stale over runs.

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;

It wouldn't work with alloca, maybe with malloc!

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 it might/might not work:

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

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

allocates a new value to binary_filename:
        binary_filename = (char *)(alloca(len) + 1);
or      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);

alloca():Here memory for binary_filename is freed: Bad!
Not the case if malloc is used! 

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.

Question: 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*)(alloca(len) + 1);
or  binary_filename = (char*)(malloc(len) + 1);

A new pointer for binary_filename is created: Is realloc usable?

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

}
alloca():Here memory for binary_filename is freed: Bad!
Not the case if malloc is used! 

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