Comment 1 Claudiu Soroiu 2010-12-22 09:00:38 EST

i was getting the same thing with 3.7M3.

Comment 2 Ayushman Jain 2010-12-22 11:52:27 EST

Incidentally, I noticed a scope for an AIOOBE in that part of the code just yesterday. But wasnt sure if there can indeed be a case which could result in one. I think I have a fair idea of how this can be fixed. I'll provide a patch here in some time. It'll be great if you can apply it and then see if it works for you. Or you can simply test the next N build.

Comment 3 Ayushman Jain 2010-12-22 12:00:26 EST

I guess this can happen when we are trying to reset the null info for a local variable that has not yet been encountered in the analysis. Thats why the extra vector hasn't grown enough to accomodate it, and now we're trying to reset a bit that doesnt exist.

I think a simple check 

if (this.extra == null || vectorIndex >= this.extra[2].length)
   return;

in UnconditionalFlowInfo.resetNullInfo(..) should work here and prevent the AIOOBE, as well as a possible NPE.
Stephan, what do you think?

Comment 4 Ayushman Jain 2010-12-22 14:55:30 EST

Created attachment 185732 [details]
proposed fix

Comment 5 Stephan Herrmann 2010-12-22 15:37:19 EST

(In reply to comment #3)
> I guess this can happen when we are trying to reset the null info for a local
> variable that has not yet been encountered in the analysis. Thats why the extra
> vector hasn't grown enough to accomodate it, and now we're trying to reset a
> bit that doesnt exist.
> 
> I think a simple check 
> 
> if (this.extra == null || vectorIndex >= this.extra[2].length)
>    return;
> 
> in UnconditionalFlowInfo.resetNullInfo(..) should work here and prevent the
> AIOOBE, as well as a possible NPE.
> Stephan, what do you think?

So this is the root cause and fix regarding bug 247564 comment 19?
From a quick glance this looks good to me.

Comment 6 Ayushman Jain 2010-12-23 05:31:06 EST

(In reply to comment #5)
> [..]
> So this is the root cause and fix regarding bug 247564 comment 19?
> From a quick glance this looks good to me.

Yeah, that's right. I guess I'll go ahead and release it by end of today.

Comment 7 Olivier Thomann 2011-01-10 12:50:02 EST

Ayushman, could you please release the fix for tomorrow's I-build?
Thanks.

Comment 8 Ayushman Jain 2011-01-11 01:11:22 EST

(In reply to comment #7)
> Ayushman, could you please release the fix for tomorrow's I-build?
> Thanks.

I was waiting for bug 247564, since this fix is also part of the patch for that bug. If I release this, I'll have to redo that patch for that bug, which Stephan is in the process of reviewing. So its better to wait a couple of days.

Comment 9 Ayushman Jain 2011-01-18 06:07:57 EST

Created attachment 186988 [details]
proposed fix v1.1 + regression tests

Same fix as above in resetNullInfo. Extended the handling of error scenarios (extra array being null or crrent index being greater than array length) to markPotentiallyNullBit(), markPotentiallyUnknownBit(), markPotentiallyNonNullBit() and implemented them in NullInfoRegistry (since here is when the erreneous condition can happen).
Added test in NullReferenceTest

Comment 10 Stephan Herrmann 2011-01-18 10:12:43 EST

(In reply to comment #9)
> Created attachment 186988 [details]
> proposed fix v1.1 + regression tests
> 
> Same fix as above in resetNullInfo. Extended the handling of error scenarios
> (extra array being null or crrent index being greater than array length) to
> markPotentiallyNullBit(), markPotentiallyUnknownBit(),
> markPotentiallyNonNullBit() and implemented them in NullInfoRegistry (since
> here is when the erreneous condition can happen).
> Added test in NullReferenceTest

Ah, so this is where the extra changes in bug 247564 comment 25 come from :)

While the change looks good, I wonder if it'd be better to apply those
extra checks to the super methods in UnconditionalFlowInfo instead.
I couldn't find a witness for trouble in UnconditionFlowInfo, but that
seems to be based only on a fragile connection: it happens that in the
case of using an UnconditionalFlowInfo always before calling markNullStatus
on some path we call something like markAsComparedEqualToNonNull
or markAsDefinitelyAssigned, which already have the array growing.
I wouldn't, however, consider this a very robust contract to rely on.

One reason why I'm concerned about those existing methods is that
I actually introduced them and forgot about array growing.

Comment 11 Ayushman Jain 2011-01-18 10:18:28 EST

(In reply to comment #10)
> (In reply to comment #9)
> [..]
> Ah, so this is where the extra changes in bug 247564 comment 25 come from :)
> 
> While the change looks good, I wonder if it'd be better to apply those
> extra checks to the super methods in UnconditionalFlowInfo instead.
> I couldn't find a witness for trouble in UnconditionFlowInfo, but that
> seems to be based only on a fragile connection: it happens that in the
> case of using an UnconditionalFlowInfo always before calling markNullStatus
> on some path we call something like markAsComparedEqualToNonNull
> or markAsDefinitelyAssigned, which already have the array growing.
> I wouldn't, however, consider this a very robust contract to rely on.

Exactly! As far as the code stands now, UnconditionalFlowInfo seems pretty safe. And yet, while working on bug 247564, I had added these checks to the super methods in UnconditionalFlowInfo as well, to safegaurd it from future changes. So, I thought I'd just skip those for this bug, and address the only the cause in the NullInfoRegistry. 
Do you think we'd better include the part in UnconditionalFlowInfo in this bug itself, or ok to leave in the patch for bug 247564?

Comment 12 Stephan Herrmann 2011-01-18 10:30:08 EST

(In reply to comment #11)
> Exactly! As far as the code stands now, UnconditionalFlowInfo seems pretty
> safe. And yet, while working on bug 247564, I had added these checks to the
> super methods in UnconditionalFlowInfo as well, to safegaurd it from future
> changes. So, I thought I'd just skip those for this bug, and address the only
> the cause in the NullInfoRegistry. 
> Do you think we'd better include the part in UnconditionalFlowInfo in this bug
> itself, or ok to leave in the patch for bug 247564?

I'd prefer to fix the array initialization/growing stuff once (and for all?)
in this bug. And we wouldn't at all need the markPotentially.. methods in 
NullInforRegistry, no?
Wouldn't that also make the patch in bug 247564 easier to understand?

But, as long as everything is fixed by the sum of these two bugs, it's fine
I guess :)

Comment 13 Ayushman Jain 2011-01-18 10:33:25 EST

(In reply to comment #12)
> [..]
> I'd prefer to fix the array initialization/growing stuff once (and for all?)
> in this bug.
Ok, lemme put up a new patch here then.

> And we wouldn't at all need the markPotentially.. methods in 
> NullInforRegistry, no?
Nope. Actually, for NullInfoRegistry we dont have to deal with the indexes 0 and 1 since theyre only for initialization info. But in UnconditionalFlowInfo, we have to deal with these bits too. So, although the difference is subtle, it cant be properly handled simply in UnconditionalFlowInfo.

Comment 14 Ayushman Jain 2011-01-18 12:09:46 EST

Created attachment 187020 [details]
proposed fix v1.2 + regression tests

This patch also adds the safeguards to UnconditionalFlowInfo.mark... methods

Comment 15 Olivier Thomann 2011-01-18 12:35:10 EST

Ayushman, please update the title to something more meaningful.
Thanks.

Comment 16 Ayushman Jain 2011-01-19 00:31:14 EST

Released in HEAD for 3.7M5

Comment 17 Ayushman Jain 2011-01-19 01:45:35 EST

Created attachment 187078 [details]
patch actually released

This is the patch that actually got released. I'd put the older patch by mistake as v1.2 again.

Comment 18 Satyam Kandula 2011-01-25 06:23:27 EST

Verified for 3.7M5 using build I20110124-1800

Comment 19 Claudiu Soroiu 2011-02-02 03:24:27 EST

I confirm that the issue is fixed in 3.7M5 that i just installed.
I do not get he AIOOBE anymore for that target branch.