Opened 4 years ago

Last modified 4 years ago

#10931 assigned bug

Services Kit: No locking around access to HTTP context information such as authentication

Reported by: stippi Owned by: pulkomandy
Priority: high Milestone: R1
Component: Kits/Network Kit Version: R1/Development
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: no Platform: All

Description

As time permits, I am trying to give the HTTP portion of the Services Kit a code review. Since I am not sure whether I find the time to investigate the problems I find more closely, or even fix them, I want to document my findings.

I think I have found one serious problem so far: There seems to be no locking around accessing the authentication per URL in the global HTTP context. This means there are several race conditions that can lead to memory corruption. There needs to be locking for accessing the hashmap of authentications per URL. And the authentication needs to be accessed via reference counting, so that the authentication object obtained in a HTTP thread can be used without the chance of it going away. This can happen when another HTTP thread replaces the authentication for a given URL. The old one needs to stay valid until all references are released.

Change History (6)

comment:1 Changed 4 years ago by pulkomandy

At least one more issue: the BUrlContext itself may be deleted while UrlRequests are still referencing it (unless the default context is used, this one is global static). Making it reference counted, and having the requests acquire a reference, would help with that.

Is it ok to use BReferenceable in public APIs? This sounds like a good way to handle BUrlContext and BHttpAuthentication lifetime.

comment:2 Changed 4 years ago by pulkomandy

Owner: changed from axeld to pulkomandy
Status: newassigned

comment:3 Changed 4 years ago by waddlesplash

Can this be fixed in time for R1/alpha5?

comment:4 Changed 4 years ago by stippi

For me personally, the crashes in the HTTP kit would be reason enough not to release. This issue cannot be the only one causing memory corruption, since HaikuDepot crashes since it uses the Kit, but it does not spawn concurrent HTTP requests. PulkoMandy seems to be working on it as we speak. As for myself - I have a wedding comming up and can't promise to find some time in the next weeks.

comment:5 in reply to:  1 Changed 4 years ago by stippi

Replying to pulkomandy:

Is it ok to use BReferenceable in public APIs? This sounds like a good way to handle BUrlContext and BHttpAuthentication lifetime.

My personal opinion is that using it is long overdue. I've used it extensively in the new text stuff I designed for HaikuDepot and plan on proposing to be public API eventually.

comment:6 Changed 4 years ago by axeld

The whole API is experimental, anyway, so just make it public (if not already), and use it.

Note: See TracTickets for help on using tickets.