Another loop in tdb.c detected

[Jeremy Allison]

On Thu, Jun 16, 2005 at 09:00:50AM +0300, Shlomi Yaakobovich wrote:
> Hi all,
> 
> I have encountered another loop in a .tdb file, a patch is attached (based on 3.0.14a). The file that caused this loop is also attached.
> 
> Shlomi

Thanks. Can you split out this patch :

                if (want_next) {
-                       /* We have offset of old record: grab next */
-                       if (rec_read(tdb, tlock->off, rec) == -1)
+                       /* We have offset of old record: grab next - just verify it's not a loop
+first*/
+                       if (rec_read(tdb, tlock->off, rec) == -1 || tlock->off == rec->next)
                                goto fail;
                        tlock->off = rec->next;
                }

to be a little clearer. ie. would the following still be correct (I think so) ? :

	if (want_next) {
		if (rec_read(tdb, tlock->off, rec) == -1)
			goto fail;

		if (tlock->off == rec->next)
			goto fail;

		tlock->off = rec->next;
	}

The reason is that the || operator evaluates left to right, but it's
sometimes hard to remember that and I don't want anyone to get confused
about which order things are done. Also, I'm a little confused as to
why your previous patch to detect infinite loops didn't catch this.

The code goes on :

                /* Iterate through chain */
                while( tlock->off) {
                        tdb_off current;
                        if (rec_read(tdb, tlock->off, rec) == -1)
                                goto fail;
                        if (!TDB_DEAD(rec)) {
                                /* Woohoo: we found one! */
                                if (lock_record(tdb, tlock->off) != 0)
                                        goto fail;
                                return tlock->off;
                        }

                        /* Detect infinite loops. From "Shlomi Yaakobovich" <Shlomi at exanet.com>. */
                        if (tlock->off == rec->next) {
                                TDB_LOG((tdb, 0, "tdb_next_lock: loop detected.\n"));
                                goto fail;
                        }

Shouldn't your "if (tlock->off == rec->next)" check in this
code segment have caught this already ?

If you could be a little clearer on explaining this I'd appreciate it.

Thanks,

	Jeremy.