[PATCH] Solve PATH_MAX issues in rrf_graph.{c, h} and rrd_tool.c

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

[PATCH] Solve PATH_MAX issues in rrf_graph.{c, h} and rrd_tool.c

Svante Signell
Hello,

Attached is an updated Debian patch for 1.4.7-2, from 2009, to avoid
PATH_MAX problems for GNU/Hurd in rrd_graph.{c,h} and rrd_tool.c. This
patch is conditioned on if MAXPATH (and __GLIBC__) is defined or not.

I would suggest to avoid PATH_MAX (MAXPATH) if possible to maximize
portability and reduce code cluttering. Depending on your decision, the
rest of my patches will be conditioned on PATH_MAX or not (there are
also a number of other Debian patches pending).

Thanks,
Svante

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

bts530814-hurd.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Solve PATH_MAX issues in rrf_graph.{c, h} and rrd_tool.c

oetiker
Administrator
Hi Svante,

Yesterday Svante Signell wrote:

> Hello,
>
> Attached is an updated Debian patch for 1.4.7-2, from 2009, to avoid
> PATH_MAX problems for GNU/Hurd in rrd_graph.{c,h} and rrd_tool.c. This
> patch is conditioned on if MAXPATH (and __GLIBC__) is defined or not.
>
> I would suggest to avoid PATH_MAX (MAXPATH) if possible to maximize
> portability and reduce code cluttering. Depending on your decision, the
> rest of my patches will be conditioned on PATH_MAX or not (there are
> also a number of other Debian patches pending).

that patch looks pretty neat I think ...

would you integrate it in your upcoming patch ?

cheers
tobi

>
> Thanks,
> Svante
>

--
Tobi Oetiker, OETIKER+PARTNER AG, Aarweg 15 CH-4600 Olten, Switzerland
http://it.oetiker.ch [hidden email] ++41 62 775 9902 / sb: -9900

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

Re: [PATCH] Solve PATH_MAX issues in rrd_graph.{c, h} and rrd_tool.c

Svante Signell
On Fri, 2013-08-16 at 09:08 +0200, Tobias Oetiker wrote:

> Hi Svante,
>
> Yesterday Svante Signell wrote:
>
> > Hello,
> >
> > Attached is an updated Debian patch for 1.4.7-2, from 2009, to avoid
> > PATH_MAX problems for GNU/Hurd in rrd_graph.{c,h} and rrd_tool.c. This
> > patch is conditioned on if MAXPATH (and __GLIBC__) is defined or not.
> >
> > I would suggest to avoid PATH_MAX (MAXPATH) if possible to maximize
> > portability and reduce code cluttering. Depending on your decision, the
> > rest of my patches will be conditioned on PATH_MAX or not (there are
> > also a number of other Debian patches pending).
>
> that patch looks pretty neat I think ...
>
> would you integrate it in your upcoming patch ?

So you wan one megapatch for PATH_MAX/MAXPATH issues? Isn't it better to
create smaller patches that can easily be reverted if something goes
wrong?

Another issue is if code should be #ifdef-ed or unconditional, e.g.
using
#ifdef MAX_PATH or
#ifndef MAX_PATH
...
#else
...
#endif
or completely removing the PATH_MAX (MAXPATH) dependency. What about the
Win32 port? I haven't looked into that.

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: [PATCH] Solve PATH_MAX issues in rrd_graph.{c, h} and rrd_tool.c

oetiker
Administrator
Hi Svante,

Today Svante Signell wrote:

> On Fri, 2013-08-16 at 09:08 +0200, Tobias Oetiker wrote:
> > Hi Svante,
> >
> > Yesterday Svante Signell wrote:
> >
> > > Hello,
> > >
> > > Attached is an updated Debian patch for 1.4.7-2, from 2009, to avoid
> > > PATH_MAX problems for GNU/Hurd in rrd_graph.{c,h} and rrd_tool.c. This
> > > patch is conditioned on if MAXPATH (and __GLIBC__) is defined or not.
> > >
> > > I would suggest to avoid PATH_MAX (MAXPATH) if possible to maximize
> > > portability and reduce code cluttering. Depending on your decision, the
> > > rest of my patches will be conditioned on PATH_MAX or not (there are
> > > also a number of other Debian patches pending).
> >
> > that patch looks pretty neat I think ...
> >
> > would you integrate it in your upcoming patch ?
>
> So you wan one megapatch for PATH_MAX/MAXPATH issues? Isn't it better to
> create smaller patches that can easily be reverted if something goes
> wrong?

many small patches are fine ... I was just asking, whether you were
plannig to build on this patch

>
> Another issue is if code should be #ifdef-ed or unconditional, e.g.
> using
> #ifdef MAX_PATH or
> #ifndef MAX_PATH
> ...
> #else
> ...
> #endif
> or completely removing the PATH_MAX (MAXPATH) dependency. What about the
> Win32 port? I haven't looked into that.

well, if there IS a limit to the path length by the OS, the code
should observe it I think, or how would you deal with this ?

cheers
tobi


> Thanks,
> Svante
>
>

--
Tobi Oetiker, OETIKER+PARTNER AG, Aarweg 15 CH-4600 Olten, Switzerland
http://it.oetiker.ch [hidden email] ++41 62 775 9902 / sb: -9900

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

Re: [PATCH] Solve PATH_MAX issues in rrd_graph.{c, h} and rrd_tool.c

Svante Signell
On Fri, 2013-08-16 at 11:15 +0200, Tobias Oetiker wrote:
> Hi Svante,
>
> Today Svante Signell wrote:
>
> > On Fri, 2013-08-16 at 09:08 +0200, Tobias Oetiker wrote:
> > > Hi Svante,
> > >

> > So you wan one megapatch for PATH_MAX/MAXPATH issues? Isn't it better to
> > create smaller patches that can easily be reverted if something goes
> > wrong?
>
> many small patches are fine ... I was just asking, whether you were
> plannig to build on this patch

OK, I will continue next with rrd_client.c. Is that one built into the
library librrd*.so* ? Is there an easy way to test with valgrind, as
rrd_deamon.c is, issuing suitable commands?

One issue with rrd_client:get_path is the const definitions. In order to
free the dynamically allocated strings, to avoid memory leaks const has
to be abandoned in some places. OK?

> > Another issue is if code should be #ifdef-ed or unconditional, e.g.
> > using
> > #ifdef MAX_PATH or
> > #ifndef MAX_PATH
> > ...
> > #else
> > ...
> > #endif
> > or completely removing the PATH_MAX (MAXPATH) dependency. What about the
> > Win32 port? I haven't looked into that.
>
> well, if there IS a limit to the path length by the OS, the code
> should observe it I think, or how would you deal with this ?

What about removing the #ifdef MAXPATH construct from the patch? By
dynamically allocating strings the only limitation is by malloc. That
should work also for win32?

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: [PATCH] Solve PATH_MAX issues in rrd_graph.{c, h} and rrd_tool.c

oetiker
Administrator
Hi Svante,

Today Svante Signell wrote:

> On Fri, 2013-08-16 at 11:15 +0200, Tobias Oetiker wrote:
> > Hi Svante,
> >
> > Today Svante Signell wrote:
> >
> > > On Fri, 2013-08-16 at 09:08 +0200, Tobias Oetiker wrote:
> > > > Hi Svante,
> > > >
>
> > > So you wan one megapatch for PATH_MAX/MAXPATH issues? Isn't it better to
> > > create smaller patches that can easily be reverted if something goes
> > > wrong?
> >
> > many small patches are fine ... I was just asking, whether you were
> > plannig to build on this patch
>
> OK, I will continue next with rrd_client.c. Is that one built into the
> library librrd*.so* ?

yes, but it can also be used standalone

> Is there an easy way to test with valgrind, as
> rrd_deamon.c is, issuing suitable commands?

nothing ready-made

> One issue with rrd_client:get_path is the const definitions. In order to
> free the dynamically allocated strings, to avoid memory leaks const has
> to be abandoned in some places. OK?

I can only tell you when I see the patch ...

> > > Another issue is if code should be #ifdef-ed or unconditional, e.g.
> > > using
> > > #ifdef MAX_PATH or
> > > #ifndef MAX_PATH
> > > ...
> > > #else
> > > ...
> > > #endif
> > > or completely removing the PATH_MAX (MAXPATH) dependency. What about the
> > > Win32 port? I haven't looked into that.
> >
> > well, if there IS a limit to the path length by the OS, the code
> > should observe it I think, or how would you deal with this ?
>
> What about removing the #ifdef MAXPATH construct from the patch? By
> dynamically allocating strings the only limitation is by malloc. That
> should work also for win32?

I don't know much about win32 limitations ... I was just wondering,
what would happen, if you hand a string longer than MAXPATH to a
filesystem function, but I guess they will take care of checking
their input as well ..

cheers
tobi


> thanks,
> Svante
>
>

--
Tobi Oetiker, OETIKER+PARTNER AG, Aarweg 15 CH-4600 Olten, Switzerland
http://it.oetiker.ch [hidden email] ++41 62 775 9902 / sb: -9900

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

Re: [PATCH] rrd_client.c issues

Svante Signell
On Fri, 2013-08-16 at 14:22 +0200, Tobias Oetiker wrote:
> Hi Svante,

> >
> > OK, I will continue next with rrd_client.c. Is that one built into the
> > library librrd*.so* ?
>
> yes, but it can also be used standalone

How?

> > One issue with rrd_client:get_path is the const definitions. In order to
> > free the dynamically allocated strings, to avoid memory leaks const has
> > to be abandoned in some places. OK?
>
> I can only tell you when I see the patch ...

Attached below is a stripped down example from rrd_client.c. Defining
NOT_CONST in the code and
gcc -g -Wall test_get_path.c
valgrind --leak-check=full ./a.out .
gives:

 valgrind --leak-check=full ./a.out .
==5155== Memcheck, a memory error detector
==5155== Copyright (C) 2002-2011, and GNU GPL'd, by Julian Seward et al.
==5155== Using Valgrind-3.7.0 and LibVEX; rerun with -h for copyright
info
==5155== Command: ./a.out .
==5155==
calling rrd_create
rrdc_create: filename=.
filename=.
get_path: path=.
absfilename=/my/path/to/rrdtool/rrdtool-git
rc = 0 (0=OK)
==5155==
==5155== HEAP SUMMARY:
==5155==     in use at exit: 0 bytes in 0 blocks
==5155==   total heap usage: 1 allocs, 1 frees, 4,096 bytes allocated
==5155==
==5155== All heap blocks were freed -- no leaks are possible
==5155==
==5155== For counts of detected and suppressed errors, rerun with: -v
==5155== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 4 from 4)

However uncommenting #undef NOT_CONST results in a small memory leak:

==5190==
==5190== HEAP SUMMARY:
==5190==     in use at exit: 4,096 bytes in 1 blocks
==5190==   total heap usage: 1 allocs, 0 frees, 4,096 bytes allocated
==5190==
==5190== 4,096 bytes in 1 blocks are definitely lost in loss record 1 of
1
==5190==    at 0x4C28BED: malloc
(in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5190==    by 0x4E6F5F7: realpath@@GLIBC_2.3 (canonicalize.c:79)
==5190==    by 0x400842: get_path (test_get_path.c:46)
==5190==    by 0x400905: rrdc_create (test_get_path.c:83)
==5190==    by 0x400983: rrd_create (test_get_path.c:107)
==5190==    by 0x4009F7: main (test_get_path.c:124)
==5190==
==5190== LEAK SUMMARY:
==5190==    definitely lost: 4,096 bytes in 1 blocks
==5190==    indirectly lost: 0 bytes in 0 blocks
==5190==      possibly lost: 0 bytes in 0 blocks
==5190==    still reachable: 0 bytes in 0 blocks
==5190==         suppressed: 0 bytes in 0 blocks
==5190==
==5190== For counts of detected and suppressed errors, rerun with: -v
==5190== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 4 from 4)

Heap usage it OK in both cases. What do you think, should the
memory-leak-free version be used for patching rrd_client.c (I have
problems to test this directly, see above)


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

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

Re: [PATCH] rrd_client.c issues

oetiker
Administrator
Hi Svante,

Today Svante Signell wrote:

> On Fri, 2013-08-16 at 14:22 +0200, Tobias Oetiker wrote:
> > Hi Svante,
>
> > >
> > > OK, I will continue next with rrd_client.c. Is that one built into the
> > > library librrd*.so* ?
> >
> > yes, but it can also be used standalone
>
> How?

in the sense that it has little dependence on rrdtool and the
license is chosen to allow linking non gnu projects

> > > One issue with rrd_client:get_path is the const definitions. In order to
> > > free the dynamically allocated strings, to avoid memory leaks const has
> > > to be abandoned in some places. OK?
> >
> > I can only tell you when I see the patch ...

if I am reading your code corectly, you are causing a memory leak
by makeing get_path return an allocated string instead of a
constant ... the root cause of the problem is that you are
changeing the API of get_path ...

for backward compatibility this is rather sub optimal ...

better choose a new function name and whoever is using it knows
that they have to free the data they get from the call.

cheers
tobi

> Attached below is a stripped down example from rrd_client.c. Defining
> NOT_CONST in the code and
> gcc -g -Wall test_get_path.c
> valgrind --leak-check=full ./a.out .
> gives:
>
>  valgrind --leak-check=full ./a.out .
> ==5155== Memcheck, a memory error detector
> ==5155== Copyright (C) 2002-2011, and GNU GPL'd, by Julian Seward et al.
> ==5155== Using Valgrind-3.7.0 and LibVEX; rerun with -h for copyright
> info
> ==5155== Command: ./a.out .
> ==5155==
> calling rrd_create
> rrdc_create: filename=.
> filename=.
> get_path: path=.
> absfilename=/my/path/to/rrdtool/rrdtool-git
> rc = 0 (0=OK)
> ==5155==
> ==5155== HEAP SUMMARY:
> ==5155==     in use at exit: 0 bytes in 0 blocks
> ==5155==   total heap usage: 1 allocs, 1 frees, 4,096 bytes allocated
> ==5155==
> ==5155== All heap blocks were freed -- no leaks are possible
> ==5155==
> ==5155== For counts of detected and suppressed errors, rerun with: -v
> ==5155== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 4 from 4)
>
> However uncommenting #undef NOT_CONST results in a small memory leak:
>
> ==5190==
> ==5190== HEAP SUMMARY:
> ==5190==     in use at exit: 4,096 bytes in 1 blocks
> ==5190==   total heap usage: 1 allocs, 0 frees, 4,096 bytes allocated
> ==5190==
> ==5190== 4,096 bytes in 1 blocks are definitely lost in loss record 1 of
> 1
> ==5190==    at 0x4C28BED: malloc
> (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==5190==    by 0x4E6F5F7: realpath@@GLIBC_2.3 (canonicalize.c:79)
> ==5190==    by 0x400842: get_path (test_get_path.c:46)
> ==5190==    by 0x400905: rrdc_create (test_get_path.c:83)
> ==5190==    by 0x400983: rrd_create (test_get_path.c:107)
> ==5190==    by 0x4009F7: main (test_get_path.c:124)
> ==5190==
> ==5190== LEAK SUMMARY:
> ==5190==    definitely lost: 4,096 bytes in 1 blocks
> ==5190==    indirectly lost: 0 bytes in 0 blocks
> ==5190==      possibly lost: 0 bytes in 0 blocks
> ==5190==    still reachable: 0 bytes in 0 blocks
> ==5190==         suppressed: 0 bytes in 0 blocks
> ==5190==
> ==5190== For counts of detected and suppressed errors, rerun with: -v
> ==5190== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 4 from 4)
>
> Heap usage it OK in both cases. What do you think, should the
> memory-leak-free version be used for patching rrd_client.c (I have
> problems to test this directly, see above)
>
>

--
Tobi Oetiker, OETIKER+PARTNER AG, Aarweg 15 CH-4600 Olten, Switzerland
http://it.oetiker.ch [hidden email] ++41 62 775 9902 / sb: -9900

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

Re: [PATCH] rrd_client.c issues

Svante Signell
On Mon, 2013-08-19 at 18:33 +0200, Tobias Oetiker wrote:

> > > I can only tell you when I see the patch ...
>
> if I am reading your code corectly, you are causing a memory leak
> by makeing get_path return an allocated string instead of a
> constant ... the root cause of the problem is that you are
> changeing the API of get_path ...

Yes, there is a memory leak introduced by allocing the string instead of
having a constant one. Regarding the API the only code calling
get_path() is in rrd_client.c.

> for backward compatibility this is rather sub optimal ...
>
> better choose a new function name and whoever is using it knows
> that they have to free the data they get from the call.

What about the new version, here get_path is defined differently
depending on whether POSIX.1-2008 is supported or not by #if-ing on
_POSIX_VERSION (see man realpath). However, I can use a different name
for the alloced version compared to the constant version.

Thanks,
Svante

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

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

Re: [PATCH] rrd_client.c issues

oetiker
Administrator
Hi Svante,

Today Svante Signell wrote:

> On Mon, 2013-08-19 at 18:33 +0200, Tobias Oetiker wrote:
>
> > > > I can only tell you when I see the patch ...
> >
> > if I am reading your code corectly, you are causing a memory leak
> > by makeing get_path return an allocated string instead of a
> > constant ... the root cause of the problem is that you are
> > changeing the API of get_path ...
>
> Yes, there is a memory leak introduced by allocing the string instead of
> having a constant one. Regarding the API the only code calling
> get_path() is in rrd_client.c.
>
> > for backward compatibility this is rather sub optimal ...
> >
> > better choose a new function name and whoever is using it knows
> > that they have to free the data they get from the call.
>
> What about the new version, here get_path is defined differently
> depending on whether POSIX.1-2008 is supported or not by #if-ing on
> _POSIX_VERSION (see man realpath). However, I can use a different name
> for the alloced version compared to the constant version.

I just realice that get_path is a static function, so there is no
problem 'fixing/changeing' it since it is not visible from the
outside.

sorry for the confusion

cheers
tobi

> Thanks,
> Svante
>

--
Tobi Oetiker, OETIKER+PARTNER AG, Aarweg 15 CH-4600 Olten, Switzerland
http://it.oetiker.ch [hidden email] ++41 62 775 9902 / sb: -9900

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

Re: [PATCH] rrd_client.c issues

Svante Signell
On Mon, 2013-08-19 at 20:54 +0200, Tobias Oetiker wrote:

> Hi Svante,
>
> Today Svante Signell wrote:
>
> > >
> > > better choose a new function name and whoever is using it knows
> > > that they have to free the data they get from the call.
> >
> > What about the new version, here get_path is defined differently
> > depending on whether POSIX.1-2008 is supported or not by #if-ing on
> > _POSIX_VERSION (see man realpath). However, I can use a different name
> > for the alloced version compared to the constant version.
>
> I just realice that get_path is a static function, so there is no
> problem 'fixing/changeing' it since it is not visible from the
> outside.
>
> sorry for the confusion

Should the code be conditional or not, what do you prefer?
BTW: get_abs_path in rrd_daemon.c is also static.


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

Re: [PATCH] rrd_client.c issues

oetiker
Administrator
Hi Svante,

Today Svante Signell wrote:

> On Mon, 2013-08-19 at 20:54 +0200, Tobias Oetiker wrote:
> > Hi Svante,
> >
> > Today Svante Signell wrote:
> >
> > > >
> > > > better choose a new function name and whoever is using it knows
> > > > that they have to free the data they get from the call.
> > >
> > > What about the new version, here get_path is defined differently
> > > depending on whether POSIX.1-2008 is supported or not by #if-ing on
> > > _POSIX_VERSION (see man realpath). However, I can use a different name
> > > for the alloced version compared to the constant version.
> >
> > I just realice that get_path is a static function, so there is no
> > problem 'fixing/changeing' it since it is not visible from the
> > outside.
> >
> > sorry for the confusion
>
> Should the code be conditional or not, what do you prefer?
> BTW: get_abs_path in rrd_daemon.c is also static.

non conditional code makes maintenance simpler so if you can make a
non static version work portably, I am all for it.

cheers
tobi

>
>

--
Tobi Oetiker, OETIKER+PARTNER AG, Aarweg 15 CH-4600 Olten, Switzerland
http://it.oetiker.ch [hidden email] ++41 62 775 9902 / sb: -9900

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

Re: [PATCH] rrd_client.c issues

oetiker
Administrator
Today Tobias Oetiker wrote:

> Hi Svante,
>
> Today Svante Signell wrote:
>
> > On Mon, 2013-08-19 at 20:54 +0200, Tobias Oetiker wrote:
> > > Hi Svante,
> > >
> > > Today Svante Signell wrote:
> > >
> > > > >
> > > > > better choose a new function name and whoever is using it knows
> > > > > that they have to free the data they get from the call.
> > > >
> > > > What about the new version, here get_path is defined differently
> > > > depending on whether POSIX.1-2008 is supported or not by #if-ing on
> > > > _POSIX_VERSION (see man realpath). However, I can use a different name
> > > > for the alloced version compared to the constant version.
> > >
> > > I just realice that get_path is a static function, so there is no
> > > problem 'fixing/changeing' it since it is not visible from the
> > > outside.
> > >
> > > sorry for the confusion
> >
> > Should the code be conditional or not, what do you prefer?
> > BTW: get_abs_path in rrd_daemon.c is also static.
>
> non conditional code makes maintenance simpler so if you can make a
> non static version work portably, I am all for it.

non-conditional I meant

cheers
tobi


--
Tobi Oetiker, OETIKER+PARTNER AG, Aarweg 15 CH-4600 Olten, Switzerland
http://it.oetiker.ch [hidden email] ++41 62 775 9902 / sb: -9900

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