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

[Tilghman Lesher]

> 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.

That's a good point, and I like that approach.


> 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. :)

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.


> 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?

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.


- Tilghman


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


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/20120214/189db319/attachment.htm>