GNU bug report logs - #35880
[PATCH 0/7] Lzip support for 'guix publish' and 'guix substitute'

Previous Next

Full log

View this message in rfc822 format

From: Ludovic Courtès <ludo <at> gnu.org>
To: Pierre Neidhardt <mail <at> ambrevar.xyz>
Cc: 35880-done <at> debbugs.gnu.org
Subject: [bug#35880] [PATCH 1/7] lzlib: Add 'make-lzip-input-port/compressed'.
Date: Fri, 31 May 2019 22:54:09 +0200

Hello,

Pierre Neidhardt <mail <at> ambrevar.xyz> skribis:

> Ludovic Courtès <ludo <at> gnu.org> writes:
>
>> I’m not sure I follow.  I think ‘make-lzip-input-port/compressed’
>> corresponds to Example 2 in the manual (info "(lzlib) Examples"),
>> ‘make-lzip-output-port’ corresponds to Example 1, and
>> ‘make-lzip-input-port’ corresponds to Example 4 (with the exception that
>> ‘lzread!’ doesn’t call ‘lz-decompress-finished?’, but it has other means
>> to tell whether we’re done processing input.)
>
> Example 4 is:
>
>      1) LZ_decompress_open
>      2) go to step 5 if LZ_decompress_write_size returns 0
>      3) LZ_decompress_write
>      4) if no more data to write, call LZ_decompress_finish
>      5) LZ_decompress_read
>      5a) optionally, if LZ_decompress_member_finished returns 1, read
>          final values for member with LZ_decompress_data_crc, etc.
>      6) go back to step 2 until LZ_decompress_finished returns 1
>      7) LZ_decompress_close
>
> In `lzread!', we don't call lz-decompress-finished? nor do we loop on
> lz-decompress-finished.

Indeed, we’re missing a call to ‘lz-decompress-finish’, so lzlib could
in theory think there’s still data coming, and so fail to produce more
output, possibly leading to an infinite loop.

I think the ‘lzread!’ loop should look like this (the tests still pass
with this):

--8<---------------cut here---------------start------------->8---
  (let loop ((read 0)
             (start start))
    (cond ((< read count)
           (match (lz-decompress-read decoder bv start (- count read))
             (0 (cond ((lz-decompress-finished? decoder)
                       read)
                      ((eof-object? (feed-decoder! decoder))
                       (lz-decompress-finish decoder)
                       (loop read start))
                      (else                       ;read again
                       (loop read start))))
             (n (loop (+ read n) (+ start n)))))
          (else
           read)))
--8<---------------cut here---------------end--------------->8---

That way, I believe all cases are correctly handled.

WDYT?

> This only works for decompression of single-member archive, but the
> documentation does not say that.
>
>     (match (get-bytevector-n port (lz-decompress-write-size decoder))
>       ((? eof-object? eof) eof)
>       (bv (lz-decompress-write decoder bv)))
>
>
> In the above if lz-decompress-write-size returns 0, we won't be reading
> anything (infinite loop?).

We’re calling ‘feed-decoder!’ iff ‘lz-decompress-read’ returned 0; when
that happens ‘lz-decompress-write-size’ must return a strictly positive
number.  Otherwise there’s an inconsistency.

>            (match (lz-decompress-read decoder bv start (- count read))
>              (0 (if (eof-object? (feed-decoder! decoder))
>                     read
>                     (loop read start)))
>
> I'm not sure I understand the above: if we read nothing, then we try
> again?

No: if we read *something*, we try again; if we read nothing, we return.

Thanks for your careful review, much appreciated!

Ludo’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.