BusyBox Bug and Patch Tracking
BusyBox
  

Viewing Issue Simple Details Jump to Notes ] View Advanced ] Issue History ] Print ]
ID Category Severity Reproducibility Date Submitted Last Update
0000349 [udhcp] trivial always 07-22-05 13:38 02-12-08 12:42
Reporter itmission View Status public  
Assigned To
Priority normal Resolution fixed  
Status closed  
Summary 0000349: max_leases off by one?
Description If i set a start address of .100 and an end address of .105, there are 6 leases available.

.100, .101, .102, .103, .104, .105

However, if I set max_leases to 6, I get a message in the syslog that reads:

udhcpd[682]: max_leases value (6) not sane, setting to 5 instead
Additional Information
Attached Files

- Relationships

- Notes
(0000469)
FlyingElephant
08-29-05 04:18

********** Overview **********

Many users have been bothered, one way or the other, by a message generated
by udhcpd, a sample of which follows:

"
Aug 29 07:19:19 udhcpd[462]: udhcpd (v0.9.9-pre) started
max_leases value (254) not sane, setting to 99 instead
"

We are bothered by this message because, heck, what did we do to cause
insanity.

Just so that normal folks can understand programmers in this. "Sanity
Check" is the phrase commonly used to refer to the performing of "edits" on
parameters - default or not. Thus, during a "Sanity Check", if something is
odd, they apparently defer to using the phrase "sane" sometimes. This is
misleading to end users when it is used in messages.

After looking at the code, I concluded:

1) The message is inappropriate. It is nondescript and it is not really an
error - it is more of an "edit" (aka "Sanity Checking") - the kind of edit
normally hidden and certainly not seemingly an error (LOG_ERROR is used).

2) The code contains a simple math error which drives some users nuts
because they don't see the need for max_leases in the first place and,
moreover, part of the implimentation of this feature screws with what should
be a simple thing.

Basically, the code subtracts the high IP from the low IP to determine how
many (memory) structures it needs to allocate, in this case "offer"
structures. Thus, it *wanted* the number 100 if one supplied the range
10.42.42.100 to 10.42.42.199 (100 distinct addresses).

However, there is a math error, and the number generated is 99. The code
does this:

NumIPs = Hi - Lo

If one wants to properly obtain this number mathimatically they must do this
(a strict example - more complex than sometimes needed):

NumIPs = AbsoluteOf ( Hi - Lo ) + 1

The current code is just plain wrong and results in users "losing" a
potential "offer". One of the addresses will not get used (not necessarily
the last or first). Even though they said hey, use 10.42.42.100 to
10.42.42.199 ... simply put, *one* won't get assigned; we can only service
99 PC's wherein our range said we had room for 100. (And that's all we
specified! Why won't it work!)

********** Towards A Solution **********

This is the cuprit code segment:

num_ips = ntohl(server_config.end) - ntohl(server_config.start);
if (server_config.max_leases > num_ips) {
    LOG(LOG_ERR, "max_leases value (%lu) not sane, "
    "setting to %lu instead",
    server_config.max_leases, num_ips);
    server_config.max_leases = num_ips;
}

Here is *one* solution to address the issue(s):

num_ips = ( ntohl(server_config.end) - ntohl(server_config.start) ) + 1;
if (server_config.max_leases > num_ips) {
    LOG(LOG_NOTICE, "max_leases value of %lu ignored, "
    "IP range only requires %lu",
    server_config.max_leases, num_ips);
    server_config.max_leases = num_ips;
}

(One may wish to use ABS or a variant here - that is up for further
investigation and discussion.)

Here I have fixed the math error, changed the message, and changed the log
level (from ERROR to NOTICE).

There have been some who don't see a need for max_leases, but I do - it is
an empowering option. However, the default of this/an option *should not*
interfere with normal operation. Here it does because of a coding error and
the other metioned reasons.

********** More **********

By the way, and testing validates, "max_leases" *does not* relate / interact
with static leases (i.e. ffw users use the ethers file). Therefore,
apparently, if we had 100 entries in the ethers file, we could set
max_leases to anything (lets pick 10) and yet all 100 (static) IP's would
still get assigned.

********** Tests Performed **********

1) 10.42.42.200 to 10.42.42.201; empty ethers file

"
Aug 29 07:07:17 udhcpd[462]: udhcpd (v0.9.9-pre) started
max_leases value (254) not sane, setting to 1 instead
"

PC1 set to IP by DHCP - 10.42.42.200 obtained
PC2 set to IP by DHCP - no IP obtained

2) 10.42.42.200 to 10.42.42.202; empty ethers file

"
Aug 29 07:19:19 udhcpd[462]: udhcpd (v0.9.9-pre) started
max_leases value (254) not sane, setting to 2 instead
"

PC1 set to IP by DHCP - 10.42.42.200 obtained
PC2 set to IP by DHCP - 10.42.42.201 obtained

3) 10.42.42.200 to 10.42.42.201; ethers file PC1_MAC = 10.42.42.16

PC1 set to IP by DHCP - 10.42.42.16 obtained
PC2 set to IP by DHCP - 10.42.42.201 obtained (it coulda been 200, but the
PC itself will first ask for it's last used IP)

********** Conclusion **********

Out attention was brought to this because of a message.

Our testing indicates there is an issue.

A look at the code shows the culprit.

We have procided herein one solution.

We will attempt next to post a message on the busybox site to the
appropriate contact.


Regards,
K. Jay Rogozinsky
 
(0000472)
FlyingElephant
08-30-05 09:51

We have been working with this in floppyfw newgroups, here are more current suggestions. There are two. The first may be more "comfortable" for the developers to consider; the second is ideal. Of course, to a good C coder, slight tweaks may be in order.

********** 1 **********

num_ips = ( ntohl(server_config.end) - ntohl(server_config.start) ) + 1;

if (server_config.max_leases < num_ips) {
    LOG(LOG_NOTICE, "max_leases will restrict the lease count to %lu"
    , server_config.max_leases);
}

if (server_config.max_leases > num_ips) {
    server_config.max_leases = num_ips;
}

You will notice that the message has been "moved". We felt that the message means little in the other "place" and means much in this "place". Also, we have removed the reference to IP usage because that could be "lying" (don't forget static allocation). These comments apply to method (2) also.

********** 2**********

leases_size = ( ntohl(server_config.end) - ntohl(server_config.start) ) + 1

if ( server_config.max_leases < leases_size )
{
    leases_size = server_config.max_leases
    LOG(LOG_NOTICE,
        "max_leases will restrict the lease count to %lu",
        server_config.max_leases);
}

Here, we get the luxury of leaving server_config.max_leases alone and thus alleviating confusion in future reading / understanding of the code.

Now:

1) num_ips gets removed from code.
2) leases_size gets added as a global unsigned long.
3) server_config.max_leases, where it is *used* (in me thinks 3 places - one just after this code block and two in other source files), gets changed to leases_size.

Regards,
K. Jay Rogozinsky et al. at the FloppyFW newsgroups
 
(0000569)
FlyingElephant
09-23-05 20:17
edited on: 09-23-05 20:46

This issue has been "addressed" ...

It was fixed about three weeks ago by "landley". S(he) said "Off by one error in max_leases sanity check. Bug 349, apparently."

S(he) *did not* alter the error message.

It's revision 11305. *Documented* here: http://www.busybox.net/cgi-bin/viewcvs.cgi/trunk/busybox/networking/udhcp/dhcpd.c?rev=11305&view=auto [^]

I don't think it's the ideal solution - don't think they even read the supplied material heretofore - shame.

 
(0004254)
vda
02-12-08 12:42

Apparently fixed in svn:

        /* Sanity check */
        num_ips = server_config.end_ip - server_config.start_ip + 1; <==== +1
        if (server_config.max_leases > num_ips) {
                bb_error_msg("max_leases=%u is too big, setting to %u",
                        (unsigned)server_config.max_leases, num_ips);
                server_config.max_leases = num_ips;
        }
 

- Issue History
Date Modified Username Field Change
07-22-05 13:38 itmission New Issue
08-29-05 04:17 FlyingElephant Issue Monitored: FlyingElephant
08-29-05 04:18 FlyingElephant Note Added: 0000469
08-30-05 09:51 FlyingElephant Note Added: 0000472
09-23-05 20:17 FlyingElephant Note Added: 0000569
09-23-05 20:18 FlyingElephant Issue End Monitor: FlyingElephant
09-23-05 20:45 FlyingElephant Note Edited: 0000569
09-23-05 20:46 FlyingElephant Note Edited: 0000569
02-12-08 12:42 vda Status new => closed
02-12-08 12:42 vda Note Added: 0004254
02-12-08 12:42 vda Resolution open => fixed


Copyright © 2000 - 2006 Mantis Group
Powered by Mantis Bugtracker