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

[jrose]

> On Feb. 14, 2012, 11:16 a.m., rmudgett wrote:
> > /branches/1.8/res/res_odbc.c, lines 555-557
> > <https://reviewboard.asterisk.org/r/1719/diff/2/?file=23941#file23941line555>
> >
> >     obj is never NULL here

Got it.


> On Feb. 14, 2012, 11:16 a.m., rmudgett wrote:
> > /branches/1.8/include/asterisk/res_odbc.h, line 47
> > <https://reviewboard.asterisk.org/r/1719/diff/2/?file=23940#file23940line47>
> >
> >     Since odbc_obj is an ao2 object, the lock should have been removed and the ao2 object lock should have been used instead.  On trunk this would be a good candidate to convert to using the ao2 rwlock when I ever get around to merging that feature. :)
> 
> Tilghman Lesher wrote:
>     It was actually kept this way to keep the API consistent across major releases.  In any case, this change should not happen in the 1.8 branch, to keep the release API consistent.  This review should be updated to apply only to trunk.

For now I've converted it to use ao2_lock.  If you think we should hold off until you have ao2_rwlock merged into trunk, we can do that too, but it seems to be working as it is right now, at least for the situations I've been testing.


> On Feb. 14, 2012, 11:16 a.m., rmudgett wrote:
> > /branches/1.8/res/res_odbc.c, line 628
> > <https://reviewboard.asterisk.org/r/1719/diff/2/?file=23941#file23941line628>
> >
> >     Extraneous blank line

got it.


> On Feb. 14, 2012, 11:16 a.m., rmudgett wrote:
> > /branches/1.8/res/res_odbc.c, lines 763-770
> > <https://reviewboard.asterisk.org/r/1719/diff/2/?file=23941#file23941line763>
> >
> >     There is no memory that the wrlock is held on the object.  There are a few cases where the sanity check is already wrlocked from the last time the sanity check was called.  So upgrading from a wrlock to a wrlock is rather pointless.
> >     
> >     A wrlock/unlock helper function could be created that sets a wrlock_held flag on the object when the wrlock is obtained and clears the flag when the lock is released.
> >     
> >     Using the wrlock_held flag would remove the need for the different ast_odbc_sanity_check calls.
> 
> Tilghman Lesher wrote:
>     That's a good point, and I like that approach.

I'll keep this in mind if we go back to read/write locks later.


> On Feb. 14, 2012, 11:16 a.m., rmudgett wrote:
> > /branches/1.8/res/res_odbc.c, line 1552
> > <https://reviewboard.asterisk.org/r/1719/diff/2/?file=23941#file23941line1552>
> >
> >     I wonder if con needs to be initialized to NULL for safety or does SQLAllocHandle always set the con value?
> 
> Tilghman Lesher wrote:
>     As long as it was handed a valid env handle (for which allocation succeeds as long as it can calloc()), then SQLAllocHandle will always initialize con.  If SQLAllocHandle on the env can't calloc(), then we're going to fail spectacularly anyway.

I'm guessing there isn't any harm in initializing it to NULL anyway.


> On Feb. 14, 2012, 11:16 a.m., rmudgett wrote:
> > /branches/1.8/res/res_odbc.c, line 1534
> > <https://reviewboard.asterisk.org/r/1719/diff/2/?file=23941#file23941line1534>
> >
> >     obj->con should be just con here because we set obj->con to NULL earlier.

Done.


- jrose


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


On Feb. 24, 2012, 2:02 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1719/
> -----------------------------------------------------------
> 
> (Updated Feb. 24, 2012, 2:02 p.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 356521 
> 
> 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/20120224/8b8d3275/attachment-0001.htm>