[Home]

Summary:ASTERISK-17453: Free parking slot logic broken if randomising start - stops early and doesn't recognize it failed (#16428 is not complete fix)
Reporter:David Woolley (davidw)Labels:
Date Opened:2011-02-21 12:52:54.000-0600Date Closed:2011-05-19 13:36:40
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Features/Parking
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:
Description:ASTERISK-15300 fixes a bug relating to the exit conditions for the free parking slot search, but only does so if the start of the search is not randomised (R option on ParkAndAnnounce()).

It stops the search when it hits the end of the range, not when it has wrapped back to the starting point.

The subsequent code only properly handles the no free slot case when it stops after going right round.  I haven't followed through, but I think there is a potential crash downstream.

****** ADDITIONAL INFORMATION ******

Also true for 1.6.2

This was noted when we rediscovered the ASTERISK-15300 bug in an earlier version and looked at the code with a view to back-porting, which we will probably still do, as we are using AMI, which doesn't support RANDOMISE.

This is the problem code:

114655    russell               for (i = start; 1; i++) {
116297    jpeeler                       if (i == parkinglot->parking_stop + 1) {
116297    jpeeler                               i = parkinglot->parking_start - 1;
238586    jpeeler                               break;
114655    russell                       }

start is not the same as parkinglot->parking_start, if randomisation is used, so this will not scan from parkingslot->parking_start, up to start - 1.

After the loop, this test is done:

114655    russell               if (i == start - 1 && cur) {
36055        oej                       ast_log(LOG_WARNING, "No more parking sp

Which will not trigger if start and parking_start are different, resulting in a false free slot of some sort.
Comments:By: Leif Madsen (lmadsen) 2011-05-18 10:18:41

OK I'm confused (and so is jrose) about this 'R' option...

"...but only does so if the start of the search is not randomised (R option on ParkAndAnnounce())."

We can't seem to find this option, how it would be used, and where it is specified. Could you enlighten us?

Thanks!
By: David Woolley (davidw) 2011-05-18 10:35:27

SVN trunk:

if (ast_test_flag(args, AST_PARK_OPT_RANDOMIZE)) {


AST_APP_OPTIONS(park_call_options, BEGIN_OPTIONS
AST_APP_OPTION('r', AST_PARK_OPT_RINGING),
AST_APP_OPTION('R', AST_PARK_OPT_RANDOMIZE),  <<<<<<<<<<
AST_APP_OPTION('s', AST_PARK_OPT_SILENCE),
END_OPTIONS );

<option name="R">
<para>Randomize the selection of a parking space.</para>
</option>
By: Jonathan Rose (jrose) 2011-05-18 12:17:58

That's an option for Park.  Not for ParkAndAnnounce.

Can you show me relevant parts of the dialplan and features.conf?  Some CLI output for when this bug happens would be nice.
By: David Woolley (davidw) 2011-05-18 12:57:52

This was picked up when looking at the code for another reason, so there isn't actually live output or dialplan to support it.

However, the obvious failures is that, except in the degenerate case, any free parking slots below the random starting point will not be considered as being available.

However, the code will actually assume that it did find something.  As I noted, I didn't follow that through, but it looks to leave it with a pointer to something it shouldn't own.

It looks as though the issue is still there in trunk.

Basically this bug is being approached from a static analysis of the code.

I suspect the reason it doesn't show up in the field is everyone simply invokes extension 700, or the feature code, so Park is never run with the R option set.  I think a lot of people don't actually realise that 700 is just a system generated dialplan entry for Park with no options.

To try and provoke it, I think one would specify a parking lot with two (might need three) entries, then put

exten => ????,1,Park(,,,R)

in the dialplan and call it multiple times, from different devices.  To break, with three slots, the third slot has to already have been taken and random has to land on it.  On the first iteration, i will point to the third slot.  As it is filled, the list traverse will set cur.  It will then go round the loop, with i pointing beyond the end.  The first if will set i to before the beginning. and the break will terminate the for loop.  The if outside the loop will compare the second slot position with the slot before the first's position, and not match, so will not report a problem and not exit with NULL.  parking_space will be set to i, which will be one before the start of the parking lot, and the code will try to proceed with that bogus parking slot entry, even if it in use for other purposes (typically the Park call), or was found by a previous cycle of this process.

I think this failure will work with just two slots, as well.


By: Jonathan Rose (jrose) 2011-05-18 13:45:50

Using the R option seems to break the thing entirely actually.  I set a parkinglot with 3 slots ranging from 301-303 and when I'd use park(,,,,R) to park myself, I'd get parked on a myriad of random numbers from 0 - 50.  Very weird.  This is in 1.8 though if that makes any difference (probably not)

On the other hand, I had no problem parking at 2 and then parking again while keeping that alive and getting 1 or 0, so I don't think wrap around is going to cause it to skip over numbers when we do get it back to working properly.
By: Jonathan Rose (jrose) 2011-05-18 15:13:32

Alrighty, I've made a patch to fix the R option.  Feel free to review it.
https://reviewboard.asterisk.org/r/1222
By: Digium Subversion (svnbot) 2011-05-19 13:32:39

Repository: asterisk
Revision: 319866

U   branches/1.8/main/features.c

------------------------------------------------------------------------
r319866 | jrose | 2011-05-19 13:32:39 -0500 (Thu, 19 May 2011) | 11 lines

Fix Randomize option on Park()

The randomize option was generally not working like it should have at all on Park().
This patch restores intended functionality.

(closes issue ASTERISK-17453)
Reported by: davidw
Tested by: jrose

Review: https://reviewboard.asterisk.org/r/1222/

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=319866
By: Digium Subversion (svnbot) 2011-05-19 13:36:39

Repository: asterisk
Revision: 319867

_U  trunk/
U   trunk/main/features.c

------------------------------------------------------------------------
r319867 | jrose | 2011-05-19 13:36:39 -0500 (Thu, 19 May 2011) | 18 lines

Merged revisions 319866 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.8

........
 r319866 | jrose | 2011-05-19 13:32:38 -0500 (Thu, 19 May 2011) | 11 lines
 
 Fix Randomize option on Park()
 
 The randomize option was generally not working like it should have at all on Park().
 This patch restores intended functionality.
 
 (closes issue ASTERISK-17453)
 Reported by: davidw
 Tested by: jrose
 
 Review: https://reviewboard.asterisk.org/r/1222/
........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=319867