On 07/30/2013 01:17 PM, Anders Widell wrote:
> Hi Nagu, and thanks for your explanation.
>
> Yes I agree now that this looks like a false positive. However, the code
> is fairly complicated and I think it is a good candidate for refactoring
> in the 4.4 branch. ;-)
Yes but I don't think this is a regression. The code has "always" looked
like this.
/Hans
>
> The initial problem I had with understanding the loop was the unexpected
> side effect:
>
> while (i_su->list_of_susi != AVD_SU_SI_REL_NULL) {
>
> /* free all the CSI assignments */
>
> avd_compcsi_delete(cb, i_su->list_of_susi, false);
>
> /* Unassign the SUSI */
>
> m_AVD_SU_SI_TRG_DEL(cb, i_su->list_of_susi);
>
> }
>
> If you look at the code above, it is not obvious that it will terminate.
> i_su->list_of_susi is checked in the loop condition, but it is not
> modified anywhere within the loop body. Furthermore, both function calls
> inside the loop body take i_su->list_of_susi as a parameter, and since
> parameters are passed by value in C, I would not expect any of the
> functions to modify the value of the pointer i_su->list_of_susi itself.
> It would have been more natural if the functions had taken i_su as a
> parameter, then I could perfectly well expect the functions to modify
> i_su->list_of_susi. Apparently, the data structures contain "back"
> pointers that point back to the data structure they belong to, and this
> is how the functions are able to modify i_su. These back-pointers are
> essentially redundant, and I don't think it is a good design - it makes
> the code is hard to follow. Indeed, this is probably the reason why the
> static code analysis tool thinks there is a use-after-free bug here.
>
> regards,
> Anders Widell
>
> 2013-07-30 06:45, Nagendra Kumar skrev:
>>
>> Hi,
>>
>> Just briefing it so that we are all on the same page:
>>
>> Let us say, SU1 has three SIs(SI1, SI2 and SI3) assigned and
>> i_su->list_of_susi pointer points to
>>
>>
>>
>>
>>
>>
>> i_su->list_of_susi à NULL
>>
>> while (i_su->list_of_susi != AVD_SU_SI_REL_NULL) {
>>
>> /* free all the CSI assignments */
>>
>> avd_compcsi_delete(cb, i_su->list_of_susi, false);
>>
>> /* Unassign the SUSI */
>>
>> m_AVD_SU_SI_TRG_DEL(cb, i_su->list_of_susi);
>>
>> }
>>
>> First iteration of above code :
>>
>> 1.avd_compcsi_delete deletes comp csi of SUSI1 from list_of_csicomp.
>>
>> 2.m_AVD_SU_SI_TRG_DEL calls avd_susi_delete(SUSI1). After below line
>> of execution, p_su_si is null and i_su_si points to SUSI1.
>>
>> /* check the SU list to get the prev pointer */
>>
>> i_su_si = susi->su->list_of_susi;
>>
>> p_su_si = NULL;
>>
>> while ((i_su_si != NULL) && (i_su_si != susi)) {
>>
>> p_su_si = i_su_si;
>>
>> i_su_si = i_su_si->su_next;
>>
>> }
>>
>> 3.Now, the below lines of code executes.
>>
>> /* now delete it from the SU list */
>>
>> if (p_su_si == NULL) {
>>
>> susi->su->list_of_susi = susi->su_next;
>>
>> susi->su_next = NULL;
>>
>> } else {
>>
>> p_su_si->su_next = susi->su_next;
>>
>> susi->su_next = NULL;
>>
>> }
>>
>> After this line of execution, i_su->list_of_susi points to SUSI2. And
>> SUSI1->next is NULL now (Earlier SUSI1->next was set to SUSI2).
>>
>> 4.After below lines of execution, SUSI1->si is null and SUSI1->su is null.
>>
>> susi->si = NULL;
>>
>> susi->su = NULL;
>>
>> 5.The below line free SUSI1.
>>
>> free(susi);
>>
>> 6.At this state the link list is as below:
>>
>>
>>
>>
>>
>>
>> i_su->list_of_susi à NULL
>>
>> Next time, when while loop executes, SUSI2 will be deleted and after
>> third iteration, SUSI3 will be deleted and i_su->list_of_susi will be
>> null and while loop will exit.
>>
>> Let me know if any further clarification is needed.
>>
>> Thanks
>>
>> -Nagu
>>
>> -----Original Message-----
>> From: Anders Widell [mailto:anders.wid...@ericsson.com]
>> Sent: 29 July 2013 20:16
>> To: opensaf-devel@lists.sourceforge.net
>> Cc: Nagendra Kumar; Hans Feldt; praveen malviya; UABHANO
>> Subject: Re: [devel] AMF static code analysis regression in 4.2.4 and
>> 4.3.1
>>
>> I assume the side effect that modifies the variable i_su->list_of_susi
>> in the loop conditional happens at the following lines in the code you
>> sent:
>>
>> /* now delete it from the SU list */
>>
>> if (p_su_si == NULL) {
>>
>> susi->su->list_of_susi = susi->su_next;
>>
>> This only happens when p_su_si is NULL. What happens if p_su_si is not
>> NULL? Will i_su->list_of_susi have the same value also in the next
>> iteration? free(susi) is executed unconditionally at the end of the
>>
>> avd_susi_delete() function, though there are a couple of return
>> statements in some branches in the code above it.
>>
>> regards,
>>
>> Anders Widell
>>
>> The code is
>>
>> 2013-07-29 14:14, Anders Widell skrev:
>>
>> > Thanks for your analysis. I still don't understand the code, but if
>>
>> > you think this warning is a false positive I take your word for it.
>>
>> > Some additional info from the warning:
>>
>> >
>>
>> > The free happens at line 504 in file avd_siass.c:
>>
>> > free(susi);
>>
>> >
>>
>> > The dereference happens at line 1070 in file avd_csi.c:
>>
>> > while (susi->list_of_csicomp != NULL) {
>>
>> >
>>
>> > regards,
>>
>> > Anders Widell
>>
>> >
>>
>> > 2013-07-29 13:50, Nagendra Kumar skrev:
>>
>> >> Hi,
>>
>> >> i_su->list_of_susi has list of susi. m_AVD_SU_SI_TRG_DEL
>> calls avd_susi_delete, which in tern separate susi from
>> i_su->list_of_susi and deletes it.
>>
>> >>
>>
>> >> Code is below:
>>
>> >> /* now delete it from the SU list */
>>
>> >> if (p_su_si == NULL) {
>>
>> >> susi->su->list_of_susi = susi->su_next;
>>
>> >> susi->su_next = NULL;
>>
>> >> } else {
>>
>> >> p_su_si->su_next = susi->su_next;
>>
>> >> susi->su_next = NULL;
>>
>> >> }
>>
>> >>
>>
>> >> /* now delete it from the SI list */
>>
>> >> if (p_si_su == NULL) {
>>
>> >> susi->si->list_of_sisu = susi->si_next;
>>
>> >> susi->si_next = NULL;
>>
>> >> } else {
>>
>> >> p_si_su->si_next = susi->si_next;
>>
>> >> susi->si_next = NULL;
>>
>> >> }
>>
>> >>
>>
>> >> And then deletes it. This means one susi is deleted from from
>> i_su->list_of_susi. When last susi is deleted from i_su->list_of_susi,
>> then i_su->list_of_susi becomes null and it exists from 'while loop'.
>>
>> >>
>>
>> >> So, I see no problem.
>>
>> >>
>>
>> >> Let me know if any further clarifications is required.
>>
>> >>
>>
>> >> Thanks
>>
>> >> -Nagu
>>
>> >>
>>
>> >> -----Original Message-----
>>
>> >> From: Anders Widell [mailto:anders.wid...@ericsson.com]
>>
>> >> Sent: 29 July 2013 16:12
>>
>> >> To: opensaf-devel@lists.sourceforge.net
>> <mailto:opensaf-devel@lists.sourceforge.net>
>>
>> >> Cc: Nagendra Kumar; Hans Feldt; praveen malviya; UABHANO
>>
>> >> Subject: AMF static code analysis regression in 4.2.4 and 4.3.1
>>
>> >>
>>
>> >> Hi!
>>
>> >>
>>
>> >> I ran some static code analysis on the release candidates for OpenSAF
>>
>> >> 4.2.4 and 4.3.1. I got a few regressions towards 4.2.3 and 4.3.0,
>> and I need your help to analyze the following in avd_sgproc.c. The
>> warning says that i_su->list_of_susi is used after free(). It is freed
>> by m_AVD_SU_SI_TRG_DEL(), and then dereferenced by
>> avd_compcsi_delete() in the next iteration.
>>
>> >>
>>
>> >> When I look at the code, I don't understand it at all. Does this
>> loop below terminate? The loop terminates when i_su->list_of_susi is
>> NULL, but it is not modified within the loop body! If the loop
>> terminates, it must be because i_su->list_of_susi is somehow modified
>> as a side-effect of calling avd_compcsi_delete() or
>> m_AVD_SU_SI_TRG_DEL(). This is a very ugly way coding!!!
>>
>> >>
>>
>> >> Line 1398 - 1408 in osaf/services/saf/avsv/avd/avd_sgproc.c on
>> branch opensaf-4.3.x (tag 4.3.1RC1):
>>
>> >> -----------------------------------------
>>
>> >> /* Free all the SU SI assignments for all the SIs on the
>>
>> >> * the SU if there are any.
>>
>> >> */
>>
>> >>
>>
>> >> while (i_su->list_of_susi != AVD_SU_SI_REL_NULL) {
>>
>> >>
>>
>> >> /* free all the CSI assignments */
>>
>> >> avd_compcsi_delete(cb, i_su->list_of_susi, false);
>>
>> >> /* Unassign the SUSI */
>>
>> >> m_AVD_SU_SI_TRG_DEL(cb, i_su->list_of_susi);
>>
>> >> }
>>
>> >> -----------------------------------------
>>
>> >>
>>
>> >> regards,
>>
>> >> Anders Widell
>>
>> >>
>>
>> >
>>
>> > ----------------------------------------------------------------------
>>
>> > -------- See everything from the browser to the database with
>>
>> > AppDynamics Get end-to-end visibility with application monitoring from
>>
>> > AppDynamics Isolate bottlenecks and diagnose root cause in seconds.
>>
>> > Start your free trial of AppDynamics Pro today!
>>
>> > http://pubads.g.doubleclick.net/gampad/clk?id=48808831&iu=/4140/ostg.c
>>
>> > lktrk _______________________________________________
>>
>> > Opensaf-devel mailing list
>>
>> > Opensaf-devel@lists.sourceforge.net
>> <mailto:Opensaf-devel@lists.sourceforge.net>
>>
>> > https://lists.sourceforge.net/lists/listinfo/opensaf-devel
>>
>> >
>>
>> >
>>
>
------------------------------------------------------------------------------
Get your SQL database under version control now!
Version control is standard for application code, but databases havent
caught up. So what steps can you take to put your SQL databases under
version control? Why should you start doing it? Read more to find out.
http://pubads.g.doubleclick.net/gampad/clk?id=49501711&iu=/4140/ostg.clktrk
_______________________________________________
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
