RFC: [PATCH] Portability by avoiding PATH_MAX

classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|

RFC: [PATCH] Portability by avoiding PATH_MAX

Svante Signell
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.

Thanks,
Svante Signell

_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers

rrd_daemon.patch (16K) Download Attachment
rrd_client.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: RFC: [PATCH] Portability by avoiding PATH_MAX

oetiker
Administrator
Hi Svante,

cool ... have you seen that there is now the beginning of a
test-suite in the HEAD branch of on github ?

cheers
tobi

Today Svante Signell wrote:

> 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.
>
> Thanks,
> Svante Signell
>

--
Tobi Oetiker, OETIKER+PARTNER AG, Aarweg 15 CH-4600 Olten, Switzerland
www.oetiker.ch [hidden email] +41 62 775 9902

_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers
Reply | Threaded
Open this post in threaded view
|

Re: RFC: [PATCH] Portability by avoiding PATH_MAX

Svante Signell
On Tue, 2014-04-29 at 19:34 +0200, Tobias Oetiker wrote:
> Hi Svante,
>
> cool ... have you seen that there is now the beginning of a
> test-suite in the HEAD branch of on github ?

Hi Tobi,

I did run tests/alltest and tests/tune1,tune2. All tests were OK.
Anything else to try to find out if the patches are OK? Anything I can
apply with valgrind, the tests are all shell scripts. Of course set -x
etc, will work.

Attached are four Debian patches from the latest built version 1.4.8-1.
They apply to the latest git too. Maybe they are of interest to apply
upstream too?

Thanks,
Svante

_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers

implicit-decl-fix (234 bytes) Download Attachment
no-rpath-for-perl (1K) Download Attachment
no-rpath-for-ruby (441 bytes) Download Attachment
setup.py-module-name (528 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: RFC: [PATCH] Portability by avoiding PATH_MAX

Svante Signell
ping!

On Thu, 2014-05-01 at 13:31 +0200, Svante Signell wrote:

> On Tue, 2014-04-29 at 19:34 +0200, Tobias Oetiker wrote:
> > Hi Svante,
> >
> > cool ... have you seen that there is now the beginning of a
> > test-suite in the HEAD branch of on github ?
>
> Hi Tobi,
>
> I did run tests/alltest and tests/tune1,tune2. All tests were OK.
> Anything else to try to find out if the patches are OK? Anything I can
> apply with valgrind, the tests are all shell scripts. Of course set -x
> etc, will work.
>
> Attached are four Debian patches from the latest built version 1.4.8-1.
> They apply to the latest git too. Maybe they are of interest to apply
> upstream too?
>
> Thanks,
> Svante


_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers
Reply | Threaded
Open this post in threaded view
|

Re: RFC: [PATCH] Portability by avoiding PATH_MAX

oetiker
Administrator
Hi Svante,

I merged a few debian patches the other day ... was this not your
pull request ?

cheers
tobi

Today Svante Signell wrote:

> ping!
>
> On Thu, 2014-05-01 at 13:31 +0200, Svante Signell wrote:
> > On Tue, 2014-04-29 at 19:34 +0200, Tobias Oetiker wrote:
> > > Hi Svante,
> > >
> > > cool ... have you seen that there is now the beginning of a
> > > test-suite in the HEAD branch of on github ?
> >
> > Hi Tobi,
> >
> > I did run tests/alltest and tests/tune1,tune2. All tests were OK.
> > Anything else to try to find out if the patches are OK? Anything I can
> > apply with valgrind, the tests are all shell scripts. Of course set -x
> > etc, will work.
> >
> > Attached are four Debian patches from the latest built version 1.4.8-1.
> > They apply to the latest git too. Maybe they are of interest to apply
> > upstream too?
> >
> > Thanks,
> > Svante
>
>
>

--
Tobi Oetiker, OETIKER+PARTNER AG, Aarweg 15 CH-4600 Olten, Switzerland
www.oetiker.ch [hidden email] +41 62 775 9902

_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers
Reply | Threaded
Open this post in threaded view
|

Re: RFC: [PATCH] Portability by avoiding PATH_MAX

Svante Signell
On Wed, 2014-05-07 at 08:14 +0200, Tobias Oetiker wrote:
> Hi Svante,
>
> I merged a few debian patches the other day ... was this not your
> pull request ?

Thanks to the Debian patches now upstream. The other question is about
how to properly test the PATH_MAX changes in rrdclient and rrddaemon,
see below.

> > > I did run tests/alltest and tests/tune1,tune2. All tests were OK.
> > > Anything else to try to find out if the patches are OK? Anything I can
> > > apply with valgrind, the tests are all shell scripts. Of course set -x
> > > etc, will work.



_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers
Reply | Threaded
Open this post in threaded view
|

Re: RFC: [PATCH] Portability by avoiding PATH_MAX

oetiker
Administrator
Today Svante Signell wrote:

> On Wed, 2014-05-07 at 08:14 +0200, Tobias Oetiker wrote:
> > Hi Svante,
> >
> > I merged a few debian patches the other day ... was this not your
> > pull request ?
>
> Thanks to the Debian patches now upstream. The other question is about
> how to properly test the PATH_MAX changes in rrdclient and rrddaemon,
> see below.
>
> > > > I did run tests/alltest and tests/tune1,tune2. All tests were OK.
> > > > Anything else to try to find out if the patches are OK? Anything I can
> > > > apply with valgrind, the tests are all shell scripts. Of course set -x
> > > > etc, will work.

just send a pull request ... there is no other magic at my disposal
atm.

you could try to add explicit tests that fail without the patch and
work with the patch ...

cheers
tobi
>
>
>
>

--
Tobi Oetiker, OETIKER+PARTNER AG, Aarweg 15 CH-4600 Olten, Switzerland
www.oetiker.ch [hidden email] +41 62 775 9902

_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers
Reply | Threaded
Open this post in threaded view
|

Re: RFC: [PATCH] Portability by avoiding PATH_MAX

Svante Signell
On Wed, 2014-05-07 at 08:36 +0200, Tobias Oetiker wrote:
> Today Svante Signell wrote:

> > > > > I did run tests/alltest and tests/tune1,tune2. All tests were OK.
> > > > > Anything else to try to find out if the patches are OK? Anything I can
> > > > > apply with valgrind, the tests are all shell scripts. Of course set -x
> > > > > etc, will work.
>
> just send a pull request ... there is no other magic at my disposal
> atm.

How to create a pull request? I have created a branch, but the patches
are not yet committed to that branch.

> you could try to add explicit tests that fail without the patch and
> work with the patch ...

Well, the PATH_MAX patches affect all architectures, but without them
rrdtool FTBFS for Hurd. For other architectures, like Linux, the code
should work the same. With the patches memory is allocated on the heap,
without on the stack. There should be no memory leaks in either case.


_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers
Reply | Threaded
Open this post in threaded view
|

Re: RFC: [PATCH] Portability by avoiding PATH_MAX

oetiker
Administrator
Hi Svante

Today Svante Signell wrote:

> On Wed, 2014-05-07 at 08:36 +0200, Tobias Oetiker wrote:
> > Today Svante Signell wrote:
>
> > > > > > I did run tests/alltest and tests/tune1,tune2. All tests were OK.
> > > > > > Anything else to try to find out if the patches are OK? Anything I can
> > > > > > apply with valgrind, the tests are all shell scripts. Of course set -x
> > > > > > etc, will work.
> >
> > just send a pull request ... there is no other magic at my disposal
> > atm.
>
> How to create a pull request? I have created a branch, but the patches
> are not yet committed to that branch.

https://github.com/tiimgreen/github-cheat-sheet

>
> > you could try to add explicit tests that fail without the patch and
> > work with the patch ...
>
> Well, the PATH_MAX patches affect all architectures, but without them
> rrdtool FTBFS for Hurd. For other architectures, like Linux, the code
> should work the same. With the patches memory is allocated on the heap,
> without on the stack. There should be no memory leaks in either case.

ok :-) that might be a bit tricky

cheers
tobi

>
>
>

--
Tobi Oetiker, OETIKER+PARTNER AG, Aarweg 15 CH-4600 Olten, Switzerland
www.oetiker.ch [hidden email] +41 62 775 9902

_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers
Reply | Threaded
Open this post in threaded view
|

Re: RFC: [PATCH] Portability by avoiding PATH_MAX

Steve Schnepp

Le 7 mai 2014 16:10, "Tobias Oetiker" <[hidden email]> a écrit :
> Today Svante Signell wrote:

> > Well, the PATH_MAX patches affect all architectures, but without them
> > rrdtool FTBFS for Hurd. For other architectures, like Linux, the code
> > should work the same. With the patches memory is allocated on the heap,
> > without on the stack. There should be no memory leaks in either case.
>
> ok :-) that might be a bit tricky

Would it be acceptable to revert to malloc() if and only if PATH_MAX is not defined ?

That would nicely minimize the patch impact on already working platforms.

--
Steve Schnepp


_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers
Reply | Threaded
Open this post in threaded view
|

Re: RFC: [PATCH] Portability by avoiding PATH_MAX

Steven Hartland
In reply to this post by Svante Signell
----- Original Message -----
From: "Svante Signell" <[hidden email]>
To: "rrdtool dev list" <[hidden email]>
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!

    Regards
    Steve

_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers
Reply | Threaded
Open this post in threaded view
|

Re: RFC: [PATCH] Portability by avoiding PATH_MAX

Svante Signell
On Thu, 2014-05-08 at 02:05 +0100, Steven Hartland wrote:

> ----- Original Message -----
> From: "Svante Signell" <[hidden email]>
> To: "rrdtool dev list" <[hidden email]>
> 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?


_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers
Reply | Threaded
Open this post in threaded view
|

Re: RFC: [PATCH] Portability by avoiding PATH_MAX

Svante Signell
In reply to this post by Steve Schnepp
On Thu, 2014-05-08 at 02:41 +0200, Steve Schnepp wrote:

> Le 7 mai 2014 16:10, "Tobias Oetiker" <[hidden email]> a écrit :
> > Today Svante Signell wrote:
>
> > > Well, the PATH_MAX patches affect all architectures, but without
> them
> > > rrdtool FTBFS for Hurd. For other architectures, like Linux, the
> code
> > > should work the same. With the patches memory is allocated on the
> heap,
> > > without on the stack. There should be no memory leaks in either
> case.
> >
> > ok :-) that might be a bit tricky
>
> Would it be acceptable to revert to malloc() if and only if PATH_MAX
> is not defined ?
>
> That would nicely minimize the patch impact on already working
> platforms.

People are advising you to not have several code patch to ease
maintenance. Of course such a decision is up to Tobi. The reason I'm
asking here is that I would like to be sure there are no problems before
proposing the patches. They are non-trivial.

_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers
Reply | Threaded
Open this post in threaded view
|

Re: RFC: [PATCH] Portability by avoiding PATH_MAX

oetiker
Administrator
Hi Svante,

Today Svante Signell wrote:

> On Thu, 2014-05-08 at 02:41 +0200, Steve Schnepp wrote:
> > Le 7 mai 2014 16:10, "Tobias Oetiker" <[hidden email]> a écrit :
> > > Today Svante Signell wrote:
> >
> > > > Well, the PATH_MAX patches affect all architectures, but without
> > them
> > > > rrdtool FTBFS for Hurd. For other architectures, like Linux, the
> > code
> > > > should work the same. With the patches memory is allocated on the
> > heap,
> > > > without on the stack. There should be no memory leaks in either
> > case.
> > >
> > > ok :-) that might be a bit tricky
> >
> > Would it be acceptable to revert to malloc() if and only if PATH_MAX
> > is not defined ?
> >
> > That would nicely minimize the patch impact on already working
> > platforms.
>
> People are advising you to not have several code patch to ease
> maintenance. Of course such a decision is up to Tobi. The reason I'm
> asking here is that I would like to be sure there are no problems before
> proposing the patches. They are non-trivial.
since the hurd has probably a rather low penetration, and if the
code path is essentially just adding extra complexity to compensate
for a problem with a particular system, I don't see a problem
only having it active on a particular platform ... we already have
several such instances to deal with windows issues ...

cheers
tobi


>

--
Tobi Oetiker, OETIKER+PARTNER AG, Aarweg 15 CH-4600 Olten, Switzerland
www.oetiker.ch [hidden email] +41 62 775 9902

_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers
Reply | Threaded
Open this post in threaded view
|

Re: RFC: [PATCH] Portability by avoiding PATH_MAX

Svante Signell
In reply to this post by Svante Signell
On Thu, 2014-05-08 at 07:18 +0200, Svante Signell wrote:
> On Thu, 2014-05-08 at 02:05 +0100, Steven Hartland wrote:
> > ----- Original Message -----
> > From: "Svante Signell" <[hidden email]>
> > To: "rrdtool dev list" <[hidden email]>
> > Sent: Tuesday, April 29, 2014 1:35 PM
> > Subject: [rrd-developers] RFC: [PATCH] Portability by avoiding PATH_MAX
> >
> >
> > > Hello,

> > 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?
I even had test code for this function, see attachment. Unfortunately
the change did not make it into the patch. Sorry for missing to update
the patch.


_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers

test_get_abs_path.c (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: RFC: [PATCH] Portability by avoiding PATH_MAX

Svante Signell
In reply to this post by oetiker
On Thu, 2014-05-08 at 07:56 +0200, Tobias Oetiker wrote:

> Hi Svante,
>
> Today Svante Signell wrote:
>
> > On Thu, 2014-05-08 at 02:41 +0200, Steve Schnepp wrote:
> > > Le 7 mai 2014 16:10, "Tobias Oetiker" <[hidden email]> a écrit :
> > > > Today Svante Signell wrote:
> > >
> > > > > Well, the PATH_MAX patches affect all architectures, but without
> > > them
> > > > > rrdtool FTBFS for Hurd. For other architectures, like Linux, the
> > > code
> > > > > should work the same. With the patches memory is allocated on the
> > > heap,
> > > > > without on the stack. There should be no memory leaks in either
> > > case.
> > > >
> > > > ok :-) that might be a bit tricky
> > >
> > > Would it be acceptable to revert to malloc() if and only if PATH_MAX
> > > is not defined ?
> > >
> > > That would nicely minimize the patch impact on already working
> > > platforms.
> >
> > People are advising you to not have several code patch to ease
> > maintenance. Of course such a decision is up to Tobi. The reason I'm
> > asking here is that I would like to be sure there are no problems before
> > proposing the patches. They are non-trivial.
>
> since the hurd has probably a rather low penetration, and if the
> code path is essentially just adding extra complexity to compensate
> for a problem with a particular system, I don't see a problem
> only having it active on a particular platform ... we already have
> several such instances to deal with windows issues ...

Yes Hurd does not have much penetration yet, right. Hopefully that will
change in due time. I can make the code conditional but as said before,
but it makes code maintenance more complicated. When the malloc/free
solution is bug-free there is no need to distinguish between *nix*
systems, only *nix* and windows.

Additionally, I normally get my patches reviewed by the Hurd developers,
and I can do this for the PATH_MAX patches too. It is considered sloppy
coding to use PATH_MAX, see
https://www.gnu.org/software/hurd/community/gsoc/project_ideas/maxpath.html
and
http://insanecoding.blogspot.se/2007/11/pathmax-simply-isnt.html

In Debian there is an increasing number of people adhering to this too.

You either use a fixed number of elements when allocating an array on
the stack or malloc/free for the heap. I think it is worthy the effort
to get things right once and for all.

_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers
Reply | Threaded
Open this post in threaded view
|

Re: RFC: [PATCH] Portability by avoiding PATH_MAX

oetiker
Administrator
Today Svante Signell wrote:

> On Thu, 2014-05-08 at 07:56 +0200, Tobias Oetiker wrote:
> > Hi Svante,
> >
> > Today Svante Signell wrote:
> >
> > > On Thu, 2014-05-08 at 02:41 +0200, Steve Schnepp wrote:
> > > > Le 7 mai 2014 16:10, "Tobias Oetiker" <[hidden email]> a écrit :
> > > > > Today Svante Signell wrote:
> > > >
> > > > > > Well, the PATH_MAX patches affect all architectures, but without
> > > > them
> > > > > > rrdtool FTBFS for Hurd. For other architectures, like Linux, the
> > > > code
> > > > > > should work the same. With the patches memory is allocated on the
> > > > heap,
> > > > > > without on the stack. There should be no memory leaks in either
> > > > case.
> > > > >
> > > > > ok :-) that might be a bit tricky
> > > >
> > > > Would it be acceptable to revert to malloc() if and only if PATH_MAX
> > > > is not defined ?
> > > >
> > > > That would nicely minimize the patch impact on already working
> > > > platforms.
> > >
> > > People are advising you to not have several code patch to ease
> > > maintenance. Of course such a decision is up to Tobi. The reason I'm
> > > asking here is that I would like to be sure there are no problems before
> > > proposing the patches. They are non-trivial.
> >
> > since the hurd has probably a rather low penetration, and if the
> > code path is essentially just adding extra complexity to compensate
> > for a problem with a particular system, I don't see a problem
> > only having it active on a particular platform ... we already have
> > several such instances to deal with windows issues ...
>
> Yes Hurd does not have much penetration yet, right. Hopefully that will
> change in due time. I can make the code conditional but as said before,
> but it makes code maintenance more complicated. When the malloc/free
> solution is bug-free there is no need to distinguish between *nix*
> systems, only *nix* and windows.
>
> Additionally, I normally get my patches reviewed by the Hurd developers,
> and I can do this for the PATH_MAX patches too. It is considered sloppy
> coding to use PATH_MAX, see
> https://www.gnu.org/software/hurd/community/gsoc/project_ideas/maxpath.html
> and
> http://insanecoding.blogspot.se/2007/11/pathmax-simply-isnt.html
>
> In Debian there is an increasing number of people adhering to this too.
>
> You either use a fixed number of elements when allocating an array on
> the stack or malloc/free for the heap. I think it is worthy the effort
> to get things right once and for all.
ok :-) how about a pull request ?

cheers
tobi
>
>

--
Tobi Oetiker, OETIKER+PARTNER AG, Aarweg 15 CH-4600 Olten, Switzerland
www.oetiker.ch [hidden email] +41 62 775 9902

_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers
Reply | Threaded
Open this post in threaded view
|

Re: RFC: [PATCH] Portability by avoiding PATH_MAX

Svante Signell
On Thu, 2014-05-08 at 09:15 +0200, Tobias Oetiker wrote:
> Today Svante Signell wrote:
...

> > Additionally, I normally get my patches reviewed by the Hurd developers,
> > and I can do this for the PATH_MAX patches too. It is considered sloppy
> > coding to use PATH_MAX, see
> > https://www.gnu.org/software/hurd/community/gsoc/project_ideas/maxpath.html
> > and
> > http://insanecoding.blogspot.se/2007/11/pathmax-simply-isnt.html
> >
> > In Debian there is an increasing number of people adhering to this too.
> >
> > You either use a fixed number of elements when allocating an array on
> > the stack or malloc/free for the heap. I think it is worthy the effort
> > to get things right once and for all.
>
> ok :-) how about a pull request ?

Hi, the link you gave did only show how to download a pull request not
how to create one? And do you have to create a pull request using
github? I have to read some more before being able to create one.


_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers
Reply | Threaded
Open this post in threaded view
|

Re: RFC: [PATCH] Portability by avoiding PATH_MAX

Steven Hartland
In reply to this post by Svante Signell

----- Original Message -----
From: "Svante Signell" <[hidden email]>
To: "Steven Hartland" <[hidden email]>
Cc: "rrdtool dev list" <[hidden email]>
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" <[hidden email]>
>> To: "rrdtool dev list" <[hidden email]>
>> 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.

_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers
Reply | Threaded
Open this post in threaded view
|

Re: RFC: [PATCH] Portability by avoiding PATH_MAX

Svante Signell
On Thu, 2014-05-08 at 10:18 +0100, Steven Hartland wrote:
> ----- Original Message -----
> From: "Svante Signell" <[hidden email]>
> To: "Steven Hartland" <[hidden email]>
> Cc: "rrdtool dev list" <[hidden email]>
> Sent: Thursday, May 08, 2014 6:18 AM
> Subject: Re: [rrd-developers] RFC: [PATCH] Portability by avoiding PATH_MAX
>

> >> 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.

Yes, it might be so that the calling functions should do freeing memory
allocated by get_abs_path, as in the example code I sent. I'll submit
modified patch(es) in due time!

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

You can see that there are still FIXMEs in the code, they have to be
removed when a proper solution/verification is at hand...

> 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.

Thanks, this is what I wanted, a review. And I'll submit to the Hurd
developers for review too before creating a pull request.


_______________________________________________
rrd-developers mailing list
[hidden email]
https://lists.oetiker.ch/cgi-bin/listinfo/rrd-developers