[pacman-dev] BUG and PATCH: 'handle' not null after alpm_release()

Markus Meissner

19 Jan 2010 19 Jan '10

11:37 a.m.

Hi pacman dev list, I am the guy behind pyalpmm and I found a not so small bug in libalpm. As I wrote some unit-tests for pyalpmm I needed to open and close a libalpm-"session" multiple times without restarting the calling application. Something like that: 1. alpm_initialize(); 2. /** do some operation here **/ 3. alpm_release(); 4. alpm_initialize(); 5. alpm_release(); But I couldn't do this, because the alpm_initialize() call in line 4 always threw an error (errno: PM_ERR_HANDLE_NOT_NULL), which translates to: "library already initialized". This is obviously not true, because the alpm_release() function was successfully called. I checked the code and found out that this line threw the error: ASSERT(handle != NULL, RET_ERR(PM_ERR_HANDLE_NULL, -1)); What means that the handle is not correctly reseted to NULL, but the _alpm_free_handle() function, which is called by alpm_release() has a FREE(handle) inside. So here is my explanation why this happens: The handle pointer is passed to _alpm_free_handle() as a value, so the pointer value itself is copied. This works perfect for freeing the contents of the handle struct, BUT setting this handle pointer to NULL just sets the locally copied pointer to NULL, what means the original handle variable remains un-touched. So every alpm_initialize() call after the first one will throw this error, no matter if you called alpm_release() or not. So to get finally end, I fixed it and the patch is (against latest git) attached to this email. Hopefully my explanation is enough to sketch the problem. Solving it, is really easy, just change the _alpm_free_handle() function to take a pointer of a pointer of pmhandle_t as an argument, change the references to handle inside the function and slightly change the _alpm_free_handle() call in alpm_release(). That's how it is done with the attached patch... Finally I also have a question: I saw many changes in the recent git compared to the latest release. Is there an estimated release date? Thanks and Greets Markus

Attachments:

handle_not_null.patch (text/x-patch — 1.3 KB)

Show replies by date

Xavier Chantry

19 Jan 19 Jan

12:34 p.m.

New subject: [pacman-dev] BUG and PATCH: 'handle' not null after alpm_release()

On Tue, Jan 19, 2010 at 12:37 PM, Markus Meissner wrote:

...

Hi pacman dev list,

I am the guy behind pyalpmm and I found a not so small bug in libalpm.

As I wrote some unit-tests for pyalpmm I needed to open and close a libalpm-"session" multiple times without restarting the calling application. Something like that:

1. alpm_initialize(); 2. /** do some operation here **/ 3. alpm_release(); 4. alpm_initialize(); 5. alpm_release();

But I couldn't do this, because the alpm_initialize() call in line 4 always threw an error (errno: PM_ERR_HANDLE_NOT_NULL), which translates to: "library already initialized". This is obviously not true, because the alpm_release() function was successfully called.

I checked the code and found out that this line threw the error:

ASSERT(handle != NULL, RET_ERR(PM_ERR_HANDLE_NULL, -1));

What means that the handle is not correctly reseted to NULL, but the _alpm_free_handle() function, which is called by alpm_release() has a FREE(handle) inside. So here is my explanation why this happens:

The handle pointer is passed to _alpm_free_handle() as a value, so the pointer value itself is copied. This works perfect for freeing the contents of the handle struct, BUT setting this handle pointer to NULL just sets the locally copied pointer to NULL, what means the original handle variable remains un-touched. So every alpm_initialize() call after the first one will throw this error, no matter if you called alpm_release() or not.

So to get finally end, I fixed it and the patch is (against latest git) attached to this email. Hopefully my explanation is enough to sketch the problem. Solving it, is really easy, just change the _alpm_free_handle() function to take a pointer of a pointer of pmhandle_t as an argument, change the references to handle inside the function and slightly change the _alpm_free_handle() call in alpm_release(). That's how it is done with the attached patch...

What about just doing handle = NULL after the _alpm_free_handle() call ?

...

Finally I also have a question: I saw many changes in the recent git compared to the latest release. Is there an estimated release date?

Not afaik. There was a plan to include some features (mainly GPG) for the next release that no one is working on. I wonder if we should not release anyway, because there are already some nice changes.

Markus Meissner

1:09 p.m.

New subject: [pacman-dev] BUG and PATCH: 'handle' not null after alpm_release()

On 01/19/2010 01:34 PM, Xavier Chantry wrote:

...

On Tue, Jan 19, 2010 at 12:37 PM, Markus Meissner wrote:

...
Hi pacman dev list,

I am the guy behind pyalpmm and I found a not so small bug in libalpm.

As I wrote some unit-tests for pyalpmm I needed to open and close a libalpm-"session" multiple times without restarting the calling application. Something like that:

1. alpm_initialize(); 2. /** do some operation here **/ 3. alpm_release(); 4. alpm_initialize(); 5. alpm_release();

But I couldn't do this, because the alpm_initialize() call in line 4 always threw an error (errno: PM_ERR_HANDLE_NOT_NULL), which translates to: "library already initialized". This is obviously not true, because the alpm_release() function was successfully called.

I checked the code and found out that this line threw the error:

ASSERT(handle != NULL, RET_ERR(PM_ERR_HANDLE_NULL, -1));

What means that the handle is not correctly reseted to NULL, but the _alpm_free_handle() function, which is called by alpm_release() has a FREE(handle) inside. So here is my explanation why this happens:

The handle pointer is passed to _alpm_free_handle() as a value, so the pointer value itself is copied. This works perfect for freeing the contents of the handle struct, BUT setting this handle pointer to NULL just sets the locally copied pointer to NULL, what means the original handle variable remains un-touched. So every alpm_initialize() call after the first one will throw this error, no matter if you called alpm_release() or not.

So to get finally end, I fixed it and the patch is (against latest git) attached to this email. Hopefully my explanation is enough to sketch the problem. Solving it, is really easy, just change the _alpm_free_handle() function to take a pointer of a pointer of pmhandle_t as an argument, change the references to handle inside the function and slightly change the _alpm_free_handle() call in alpm_release(). That's how it is done with the attached patch...

What about just doing handle = NULL after the _alpm_free_handle() call ?

Yes also thought about that, but I don't like scattering such things around. I mean, _alpm_free_handle() should do what it's called like. So you also don't have to look at different places, if you want to know what gets freed. But honestly I don't mind a lot how, as long as this will be fixed at some point in maybe near future, because it undermines the way I like to write unit tests ;)

...

...
Finally I also have a question: I saw many changes in the recent git compared to the latest release. Is there an estimated release date?

Not afaik. There was a plan to include some features (mainly GPG) for the next release that no one is working on. I wonder if we should not release anyway, because there are already some nice changes.

I would absolutly second a release, this would help me a lot as the API changed at some points. It also would be incredibly great, if the API changes would be a little more "third-party-library" friendly. Like first marking items as deprecated for one minor release, before removing them. I think there are some people working with libalpm who would appreciate this, too - thinking about Dario Freddi for example. (Or am I just too inattentive/slow and there was a deprecation-time for things like alpm_trans_addtarget or the ALPM_TRANS_TYPEs?) greets Markus

Dan McGee

2:01 p.m.

New subject: [pacman-dev] BUG and PATCH: 'handle' not null after alpm_release()

On Tue, Jan 19, 2010 at 7:09 AM, Markus Meissner wrote:

...

On 01/19/2010 01:34 PM, Xavier Chantry wrote:

...
On Tue, Jan 19, 2010 at 12:37 PM, Markus Meissner wrote:

...
But I couldn't do this, because the alpm_initialize() call in line 4 always threw an error (errno: PM_ERR_HANDLE_NOT_NULL), which translates to: "library already initialized". This is obviously not true, because the alpm_release() function was successfully called.

So to get finally end, I fixed it and the patch is (against latest git) attached to this email. Hopefully my explanation is enough to sketch the problem. Solving it, is really easy, just change the _alpm_free_handle() function to take a pointer of a pointer of pmhandle_t as an argument, change the references to handle inside the function and slightly change the _alpm_free_handle() call in alpm_release(). That's how it is done with the attached patch...

What about just doing handle = NULL after the _alpm_free_handle() call ?

Yes also thought about that, but I don't like scattering such things around. I mean, _alpm_free_handle() should do what it's called like. So you also don't have to look at different places, if you want to know what gets freed.

I'm with Xavier on this, patch is below. We only call this in one place, and could really just inline the code in there if we want to do it right... And on the "do what it's name indicates" topic, realize that in C free() doesn't set the passed pointer to NULL; you are expected to know that value is bogus. We just didn't mark it as such on the return from our own call to a free()-like function.

...

...
...
Finally I also have a question: I saw many changes in the recent git compared to the latest release. Is there an estimated release date?

Not afaik. There was a plan to include some features (mainly GPG) for the next release that no one is working on. I wonder if we should not release anyway, because there are already some nice changes.

I would absolutly second a release, this would help me a lot as the API changed at some points. It also would be incredibly great, if the API changes would be a little more "third-party-library" friendly. Like first marking items as deprecated for one minor release, before removing them. I think there are some people working with libalpm who would appreciate this, too - thinking about Dario Freddi for example. (Or am I just too inattentive/slow and there was a deprecation-time for things like alpm_trans_addtarget or the ALPM_TRANS_TYPEs?)

This is a failure on my part; hard to blame others here. I've thought about getting it to a release state as otherwise good changes do sit around unreleased. The reason we don't do the "third party library friendly" stuff? Because we get next to zero traffic on the mailing list from you guys. Seriously, this is the first message from someone in quite some time, and all the time we pose questions to the ML asking about a API change and get zero responses. -Dan diff --git a/lib/libalpm/alpm.c b/lib/libalpm/alpm.c index 6ab6516..8250256 100644 --- a/lib/libalpm/alpm.c +++ b/lib/libalpm/alpm.c @@ -71,6 +71,7 @@ int SYMEXPORT alpm_release(void) } _alpm_handle_free(handle); + handle = NULL; return(0); }

Markus Meissner

2:29 p.m.

New subject: [pacman-dev] BUG and PATCH: 'handle' not null after alpm_release()

On 01/19/2010 03:01 PM, Dan McGee wrote:

...

On Tue, Jan 19, 2010 at 7:09 AM, Markus Meissner wrote:

...
On 01/19/2010 01:34 PM, Xavier Chantry wrote:

...
On Tue, Jan 19, 2010 at 12:37 PM, Markus Meissner wrote:

...
But I couldn't do this, because the alpm_initialize() call in line 4 always threw an error (errno: PM_ERR_HANDLE_NOT_NULL), which translates to: "library already initialized". This is obviously not true, because the alpm_release() function was successfully called.

So to get finally end, I fixed it and the patch is (against latest git) attached to this email. Hopefully my explanation is enough to sketch the problem. Solving it, is really easy, just change the _alpm_free_handle() function to take a pointer of a pointer of pmhandle_t as an argument, change the references to handle inside the function and slightly change the _alpm_free_handle() call in alpm_release(). That's how it is done with the attached patch...

What about just doing handle = NULL after the _alpm_free_handle() call ?

Yes also thought about that, but I don't like scattering such things around. I mean, _alpm_free_handle() should do what it's called like. So you also don't have to look at different places, if you want to know what gets freed.

I'm with Xavier on this, patch is below. We only call this in one place, and could really just inline the code in there if we want to do it right...

And on the "do what it's name indicates" topic, realize that in C free() doesn't set the passed pointer to NULL; you are expected to know that value is bogus. We just didn't mark it as such on the return from our own call to a free()-like function.

Alright here - Actually I have to admit that just one line is always better, than changing function prototypes and stuff^^

...

...
...
...
Finally I also have a question: I saw many changes in the recent git compared to the latest release. Is there an estimated release date?

Not afaik. There was a plan to include some features (mainly GPG) for the next release that no one is working on. I wonder if we should not release anyway, because there are already some nice changes.

I would absolutly second a release, this would help me a lot as the API changed at some points. It also would be incredibly great, if the API changes would be a little more "third-party-library" friendly. Like first marking items as deprecated for one minor release, before removing them. I think there are some people working with libalpm who would appreciate this, too - thinking about Dario Freddi for example. (Or am I just too inattentive/slow and there was a deprecation-time for things like alpm_trans_addtarget or the ALPM_TRANS_TYPEs?)

This is a failure on my part; hard to blame others here. I've thought about getting it to a release state as otherwise good changes do sit around unreleased.

The reason we don't do the "third party library friendly" stuff? Because we get next to zero traffic on the mailing list from you guys. Seriously, this is the first message from someone in quite some time, and all the time we pose questions to the ML asking about a API change and get zero responses.

-Dan

I didn't want to blame someone, and that's why I asked if I maybe missed it, because I just finished my graduation last month there was absolutly not enough time to even read the ML completly. But hopefully this will change, I just pushing the lib to work with recent git and I have to think later what to do with the stable release once this is done. Don't get me wrong. I know that all these "tplf" (third-party-library-friendly) wishes produce a not so small extra amount of work on your side and if there is no feedback from us tplf-guys I can absolutly understand that the priority for such things is low. The only thing I can say right now is that I'll try to read the ML regularly and as all those changes are discussed here, this is more than enough right now. Greets Markus

...

diff --git a/lib/libalpm/alpm.c b/lib/libalpm/alpm.c index 6ab6516..8250256 100644 --- a/lib/libalpm/alpm.c +++ b/lib/libalpm/alpm.c @@ -71,6 +71,7 @@ int SYMEXPORT alpm_release(void) }

_alpm_handle_free(handle); + handle = NULL;

return(0); }

Dario Freddi

21 Jan 21 Jan

4:18 p.m.

New subject: [pacman-dev] BUG and PATCH: 'handle' not null after alpm_release()

On Tuesday 19 January 2010 15:01:06 Dan McGee wrote:

...

The reason we don't do the "third party library friendly" stuff? Because we get next to zero traffic on the mailing list from you guys. Seriously, this is the first message from someone in quite some time, and all the time we pose questions to the ML asking about a API change and get zero responses.

This is not true _at all_. The last mail about a significant API change (unless I missed something, which might be) was the one on the download callbacks where I shared many thoughts, and I was probably the most active poster in the thread. -- ------------------- Dario Freddi KDE Developer GPG Key Signature: 511A9A3B

Dario Freddi

4:21 p.m.

New subject: [pacman-dev] BUG and PATCH: 'handle' not null after alpm_release ()

On Tuesday 19 January 2010 14:09:46 Markus Meissner wrote:

...

I would absolutly second a release, this would help me a lot as the API changed at some points. It also would be incredibly great, if the API changes would be a little more "third-party-library" friendly. Like first marking items as deprecated for one minor release, before removing them. I think there are some people working with libalpm who would appreciate this, too - thinking about Dario Freddi for example. (Or am I just too inattentive/slow and there was a deprecation-time for things like alpm_trans_addtarget or the ALPM_TRANS_TYPEs?)

Yes, it would be nice - although not very feasible with the actual pacman/alpm development model, where most API changes are decided between minor releases - hence you have no time to mark stuff as deprecated. That's at least from an user point of view. However, I never complained about this - on pacman-dev all commits are recorded and you can simply git diff out the last tag with master - and if you're developing your bindings in a git repository, like I do, it makes your life extremely easier :)

...

greets Markus

-- ------------------- Dario Freddi KDE Developer GPG Key Signature: 511A9A3B

5220

Age (days ago)

5222

Last active (days ago)

List overview

Download

6 comments

4 participants

participants (4)

Dan McGee
Dario Freddi
Markus Meissner
Xavier Chantry