[asterisk-dev] [Code Review]: Resolve odbc segfaults by adding shared locks around usage of odbc handle in res_odbc

[jrose]

> On Feb. 8, 2012, 2:33 a.m., wdoekes wrote:
> > /branches/1.8/include/asterisk/res_odbc.h, line 160
> > <https://reviewboard.asterisk.org/r/1719/diff/1/?file=23900#file23900line160>
> >
> >     +1 on the _wrlocked suffix. It beats '2' any single day.
> 
> Tilghman Lesher wrote:
>     I don't really agree with that.  The problem comes to any new developer who sees a host of different APIs and wonders which one is the latest.  Adding a '2' makes it very clear that it's a later version of the function.  I'd also prefer if the underlying 2-argument function were exposed, instead of hiding it (and thus permitting new users to pass either 1 or 0 themselves), instead of doubling the number of APIs they need to remember.
> 
> wdoekes wrote:
>     If you put it like that, sure. But that only makes sense if your second preference is met; exposing the lock_type integer. And in this case, we'd have to know which integer means what (was 0 unlocked or readlocked?), which means adding defines or enums which we have to keep track of.
>     
>     Having said that, I wonder why the function is exposed at all. No one outside res_odbc is using it, except perhaps 3rd party callers? And I believe you already pointed out that we're already breaking things for them: they won't have the read-lock on it at all.
>
> 
> Tilghman Lesher wrote:
>     Well, that creates another problem entirely.  Currently, the way the code is written, sanity_check has no lock at all, and if we require a lock, then it should acquire it in the old API function.  Separately, we should be able to indicate (in the new API) that we already have a read lock, which may need to be escalated to a write lock (with an emitted warning), and finally, we should be able to indicate that we already have a write lock, which does not need to be escalated (and thus avoid an emitted warning).
>     
>     I considered some of these issues when I wrote the code, which is why I used a mutex instead of a rwlock.  Rwlocks are great for concurrency, but they considerably complicate the code when APIs need the ability to escalate locks.  (Not to mention that if an API escalates to a write lock and returns to a caller that otherwise kept a persistent read lock, you have none of the advantages of having used rwlocks in the first place).

Actually, there probably isn't any problem with just using ast_odbc_sanity_check even if you have a write lock... at least not compared to the previous behavior.  It will just unnecessarily release the write lock so that it can get it again in that case, which is only a problem if you need a guarantee that obj isn't changed during that operation.  That guarantee wasn't in place before, so there isn't really any new disadvantage brought about by still using that wrapper and we aren't messing up unknown third party code with these changes (well, in this respect anyway).

It's definitely preferable to use the wrlocked variant though when you have a write lock of course since in that case releasing the lock is just completely unnecessary.

As for exposing the full function, I don't really know how I feel about that. Like Walter said, just because you have to remember one less function doesn't mean you don't have to remember just as much extra junk with the extra input. It also means one more extra function in the API to accomplish the same general task. I think the code is more maintainable if we have leave this as an unexposed helper function because there is less need to decipher the same function with different names and arguments as actually being the same function, and again, the audiohook read_frame function in 10+ is a recent example of this having precedent.


- jrose


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1719/#review5432
-----------------------------------------------------------


On Feb. 8, 2012, 8:47 a.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1719/
> -----------------------------------------------------------
> 
> (Updated Feb. 8, 2012, 8:47 a.m.)
> 
> 
> Review request for Asterisk Developers, Mark Michelson, Tilghman Lesher, rmudgett, and wdoekes.
> 
> 
> Summary
> -------
> 
> This is a continuation of wdoekes patch seen in https://reviewboard.asterisk.org/r/1622/
> This adds Tilghman Lesher's suggested changes.
> 
> 
> This addresses bug ASTERISK-19011.
>     https://issues.asterisk.org/jira/browse/ASTERISK-19011
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/res/res_odbc.c 354164 
>   /branches/1.8/include/asterisk/res_odbc.h 354164 
> 
> Diff: https://reviewboard.asterisk.org/r/1719/diff
> 
> 
> Testing
> -------
> 
> I started by confirming that the segfault could be easily reproduced with wdoekes' setup mentioned in the issue report.
> After finishing the changes to the patch, I confirmed that following the same steps no longer produced the segfault, undid the patch to double check that I was doing it right, and then decided it was safe to post this review.
> 
> 
> Thanks,
> 
> jrose
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120208/602b484d/attachment-0001.htm>