Bug with Allegro packfile compression

no-reply@allegro.cc (jman2050) — Tue, 22 Aug 2006 19:15:05 +0000

I'm not entirely sure if it's the compression itself or the getc/putc functions, but using a packfile to store and read data can occasionally cause our data to become corrupt. What happens is that the unpacked data gets written to a packed packfile. However, when the packfile is read via Allegro's built-in functions, the output ends up being one byte short of the original's length, which screws up our loading functions. Note that this only occurs extremely rarely.

Packed into the file below is a test program I wrote to demonstrate this problem. It contains the program, the source, the dll, and the uncompressed data from our program that's exhibiting this problem. Any help would e greatly appreciated.

http://www.cgi101.com/~jman2050/pack_test.zip

EDIT - forgot to mention that the program takes tst.tst, packs it via pack_putc, then unpacks itvia pack_getc, and puts the result into output.tst

no-reply@allegro.cc (Peter Wang) — Wed, 23 Aug 2006 03:36:52 +0000

I can confirm the bug. I'd thought there was a bug remaining in the packfile code. Thanks for the test case.

In case anyone wants to have a go at this, I've found that:

(1) the bug doesn't show up with a random file of the same length as test.tst;

(2) appending a single byte to test.tst causes the bug not to show;

(3) removing the last byte of test.tst causes the bug not to show;

(4) removing the first byte of test.tst causes the bug not to show.

no-reply@allegro.cc (Milan Mimica) — Wed, 30 Aug 2006 19:34:13 +0000

The bug also shows if you "double" test.tst by:
$ cat test.tst test.tst >tmp.tst; mv tmp.tst test.tst
Then output.tst lacks two bytes.

no-reply@allegro.cc (Peter Wang) — Wed, 30 Aug 2006 19:52:01 +0000

Going to sleep now. I'm pretty certain the compressed file is okay. I checked the length using the following program. It seems the bug is an off by one error in the read buffering.

1#include 
2#include 
3#include 
4 
5int main(void)
6{
  FILE *fp;
  int length;
9 
  fp = fopen("tmp.tmp", "rb");
  if (!fp) {
      exit(EXIT_FAILURE);
  }
14 
  /* skip signature */
  fgetc(fp);
  fgetc(fp);
  fgetc(fp);
  fgetc(fp);
20 
  length = 0;
  while (1) {
      int flags;
      int bit;
25 
      flags = fgetc(fp);
      if (flags == EOF) {
          printf("Reached EOF\n");
          break;
      }
31 
      for (bit = 0; bit < 8; bit++) {
          if (flags & (1 << bit)) {
              if (fgetc(fp) == EOF) {
                  printf("Reached EOF while reading raw byte\n");
                  goto Exit;
              }
              length++;
          }
          else {
              int byte1;
              int byte2;
              int index;
              int len;
45 
              byte1 = fgetc(fp);
              if (byte1 == EOF) {
                  printf("Reached EOF while reading byte1\n");
                  goto Exit;
              }
51 
              byte2 = fgetc(fp);
              if (byte2 == EOF) {
                  printf("Reached EOF while reading byte2\n");
                  goto Exit;
              }
57 
              index = byte1 | ((byte2 & 0xF0) << 4);
              len = (byte2 & 0x0F) + 3;
60 
              assert(index >= 0 && index < 4096);
              length += len;
          }
      }
  }
66 
Exit:
68 
  fclose(fp);
  printf("Length = %d bytes\n", length);
  return 0;
72}

EDIT: For anyone not on [AD], I posted this:

Quote:

I think I have a fix. I would appreciate help looking it over (see the
comments for detailed explanation of the problem) and stress testing it,
since Evert is planning to release 4.2.1 this weekend. In addition to
the test case in the thread, I've attached a shell script which produces
files using /dev/zero and /dev/urandom, compresses and decompresses them
with the `pack' tool and checks they are the same.

I've tried to minimise the number of changes and maintaining PACKFILE
semantics, in particular that pack_feof() returns TRUE immediately after
the last byte has been read (unlike feof() which returns true once you
try to read past the last byte).

The patch and shell script are attached.

no-reply@allegro.cc (jman2050) — Mon, 04 Sep 2006 03:38:38 +0000

The fix seems to have done the trick. I'll report if anything else goes wrong. Thanks a lot Peter

1	#include
2	#include
3	#include
4
5	int main(void)
6	{
7	FILE *fp;
8	int length;
9
10	fp = fopen("tmp.tmp", "rb");
11	if (!fp) {
12	exit(EXIT_FAILURE);
13	}
14
15	/* skip signature */
16	fgetc(fp);
17	fgetc(fp);
18	fgetc(fp);
19	fgetc(fp);
20
21	length = 0;
22	while (1) {
23	int flags;
24	int bit;
25
26	flags = fgetc(fp);
27	if (flags == EOF) {
28	printf("Reached EOF\n");
29	break;
30	}
31
32	for (bit = 0; bit < 8; bit++) {
33	if (flags & (1 << bit)) {
34	if (fgetc(fp) == EOF) {
35	printf("Reached EOF while reading raw byte\n");
36	goto Exit;
37	}
38	length++;
39	}
40	else {
41	int byte1;
42	int byte2;
43	int index;
44	int len;
45
46	byte1 = fgetc(fp);
47	if (byte1 == EOF) {
48	printf("Reached EOF while reading byte1\n");
49	goto Exit;
50	}
51
52	byte2 = fgetc(fp);
53	if (byte2 == EOF) {
54	printf("Reached EOF while reading byte2\n");
55	goto Exit;
56	}
57
58	index = byte1 \| ((byte2 & 0xF0) << 4);
59	len = (byte2 & 0x0F) + 3;
60
61	assert(index >= 0 && index < 4096);
62	length += len;
63	}
64	}
65	}
66
67	Exit:
68
69	fclose(fp);
70	printf("Length = %d bytes\n", length);
71	return 0;
72	}