[rrd-developers] RFC: [PATCH] Portability by avoiding PATH_MAX

[Steven Hartland]

----- Original Message ----- 
From: "Svante Signell" <svante.signell at gmail.com>
To: "Steven Hartland" <killing at multiplay.co.uk>
Cc: "rrdtool dev list" <rrd-developers at lists.oetiker.ch>
Sent: Thursday, May 08, 2014 6:18 AM
Subject: Re: [rrd-developers] RFC: [PATCH] Portability by avoiding PATH_MAX


> On Thu, 2014-05-08 at 02:05 +0100, Steven Hartland wrote:
>> ----- Original Message ----- 
>> From: "Svante Signell" <svante.signell at gmail.com>
>> To: "rrdtool dev list" <rrd-developers at lists.oetiker.ch>
>> Sent: Tuesday, April 29, 2014 1:35 PM
>> Subject: [rrd-developers] RFC: [PATCH] Portability by avoiding PATH_MAX
>> 
>> 
>> > Hello,
>> > 
>> > Here are updated patches for rrdtool/rrd_client.c and
>> > rrdtool/rrd_daemon.c. We had some discussions in August last year. I
>> > created a branch and the diffs are against latest git. I've run the
>> > code, especially rrd_daemon with valgrind under Linux, but need some
>> > help to check that also rrd_client works OK (maybe rrd_daemon too). Can
>> > you help me with test cases to run the execute the code modified paths.
>> > I know one application using rrdtool, lm-sensors (build-dependency on
>> > librrd2-dev), but am not sure my computers have the sensors to be a good
>> > test case.
>> > 
>> > Note that the modified functions get_path() and get_abs_path() are
>> > static, so they don't change the API.
>> 
>> This looks like it would cause a lot of unnessary malloc free surely
>> the correct fix for this is to fix the OS config to define PATH_MAX.
>> 
>> Quick look at the patches also show its quite broken:-
>> 
>> +  tmp = malloc(len);
>> +  snprintf(tmp, len, "%s/%s", config_base_dir, *filename);
>>    *filename = tmp;
>> +  free(tmp);
>> 
>> So you just malloced some memory, assigned the pointer to it then freed
>> the memory, so your now going to get a use after free and BOOM!
> 
> This is of course wrong, thanks! You see the reason for having tests
> together with valgrind to find out these problems. Did you find more?

Obviously this means that any calls to get_abs_path need reworking in the
patch.

I've not had time to go through it in detail but looks like the journal_dir
/ realpath changes also have issues.

Also there's some formatting issues with tabs instead of spacing.

    Regards
    Steve
Really needs a proper review as there's lots of potential for issues with
this.