[PATCH v2 7/7] iconv: Input buffering for the iconv program (bug 32050)
DJ Delorie
dj@redhat.com
Fri Sep 20 03:53:01 GMT 2024
Florian Weimer <fweimer@redhat.com> writes:
> Do not read the entire input file into memory.
How is this related to bz 32050?
Otherwise LGTM with the one typo fix
Reviewed-by: DJ Delorie <dj@redhat.com>
> diff --git a/iconv/iconv_prog.c b/iconv/iconv_prog.c
>
> -static void close_output_file (int status);
> +static void close_output_file (__gconv_t cd, int status);
> -static int process_block (iconv_t cd, char *addr, size_t len);
> +static int process_block (iconv_t cd, char **addr, size_t *len,
> + off64_t file_offset, bool *incomplete);
Ok.
> - close_output_file (status);
> + close_output_file (cd, status);
Ok.
> static void
> -close_output_file (int status)
> +close_output_file (__gconv_t cd, int status)
Ok.
>
> - /* The current_input_file_index variable is now larger than
> - last_overlapping_file_index, so the flush_output call switches
> + /* All the input test is processed. For state-dependent character
s/test/text/ as Andreas noted.
> + sets we have to flush the state now.
> +
> + The current_input_file_index variable is now larger than
> + last_overlapping_file_index, so the flush_output calls switch
> away from the temporary file. */
> + size_t n = iconv (cd, NULL, NULL,
> + &output_buffer_current, &output_buffer_remaining);
> + if (n == (size_t) -1 && errno == E2BIG)
> + {
> + /* Try again if the state flush exceeded the buffer space. */
> + flush_output ();
> + n = iconv (cd, NULL, NULL,
> + &output_buffer_current, &output_buffer_remaining);
> + }
> + int saved_errno = errno;
Ok.
> flush_output ();
> + if (n == (size_t) -1 && !omit_invalid)
> + {
> + errno = saved_errno;
> + output_error ();
> + }
Ok.
> +/* CD is the iconv handle. Input processing starts at *ADDR, and
> + consumes upto *LEN bytes. *ADDR and *LEN are updated. FILE_OFFSET
> + is the file offset of the data initially at ADDR. *INCOMPLETE is
> + set to true if conversion stops due to an incomplete input
> + sequence. */
> static int
> -process_block (iconv_t cd, char *addr, size_t len)
> +process_block (iconv_t cd, char **addr, size_t *len, off64_t file_offset,
> + bool *incomplete)
Ok.
> {
> - const char *start = addr;
> + const char *start = *addr;
Ok.
> - while (len > 0)
> + while (*len > 0)
Ok.
> {
> - n = iconv (cd, &addr, &len,
> + n = iconv (cd, addr, len,
> &output_buffer_current, &output_buffer_remaining);
Ok.
> if (n == (size_t) -1 && omit_invalid && errno == EILSEQ)
> {
> ret = 1;
> - if (len == 0)
> + if (*len == 0)
Ok.
> if (n != (size_t) -1)
> - {
> - /* All the input test is processed. For state-dependent
> - character sets we have to flush the state now. */
> - n = iconv (cd, NULL, NULL,
> - &output_buffer_current, &output_buffer_remaining);
> - if (n == (size_t) -1 && errno == E2BIG)
> - {
> - /* Try again if the state flush exceeded the buffer space. */
> - flush_output ();
> - n = iconv (cd, NULL, NULL,
> - &output_buffer_current, &output_buffer_remaining);
> - }
> - bool errno_is_EILSEQ = errno == EILSEQ;
> -
> - if (n != (size_t) -1)
> - break;
> -
> - if (omit_invalid && errno_is_EILSEQ)
> - {
> - ret = 1;
> - break;
> - }
> - }
> + break;
Ok. EILSEQ is handled implicitly above.
> @@ -680,13 +683,12 @@ process_block (iconv_t cd, char *addr, size_t len)
> {
> case EILSEQ:
> if (! omit_invalid)
> - error (0, 0, _("illegal input sequence at position %ld"),
> - (long int) (addr - start));
> + error (0, 0, _("illegal input sequence at position %lld"),
> + (long long int) (file_offset + (*addr - start)));
Ok.
> case EINVAL:
> - error (0, 0, _("\
> -incomplete character or shift sequence at end of buffer"));
> - break;
> + *incomplete = true;
> + return ret;
Ok.
> static int
> process_fd (iconv_t cd, int fd)
> {
> - /* we have a problem with reading from a descriptor since we must not
> - provide the iconv() function an incomplete character or shift
> - sequence at the end of the buffer. Since we have to deal with
> - arbitrary encodings we must read the whole text in a buffer and
> - process it in one step. */
> - static char *inbuf = NULL;
> - static size_t maxlen = 0;
> - char *inptr = inbuf;
> - size_t actlen = 0;
> -
> - while (actlen < maxlen)
> + char inbuf[BUFSIZ];
> + char *inbuf_end = inbuf + sizeof (inbuf);
> + size_t inbuf_used = 0;
> + off64_t file_offset = 0;
> + int status = 0;
> + bool incomplete = false;
> +
> + while (true)
> {
Ok.
> - ssize_t n = read (fd, inptr, maxlen - actlen);
> -
> - if (n == 0)
> - /* No more text to read. */
> - break;
> -
> - if (n == -1)
> + char *p = inbuf + inbuf_used;
> + ssize_t read_ret = read (fd, p, inbuf_end - p);
> + if (read_ret == 0)
> + {
> + /* On EOF, check if the previous iconv invocation saw an
> + incomplete sequence. */
> + if (incomplete)
> + {
> + error (0, 0, _("\
> +incomplete character or shift sequence at end of buffer"));
> + return 1;
> + }
> + return 0;
> + }
Ok.
> + if (read_ret < 0)
> {
> - /* Error while reading. */
> error (0, errno, _("error while reading the input"));
> return -1;
> }
Ok.
> - inptr += n;
> - actlen += n;
> + inbuf_used += read_ret;
> + incomplete = false;
> + p = inbuf;
> + int ret = process_block (cd, &p, &inbuf_used, file_offset, &incomplete);
> + if (ret != 0)
> + {
> + status = ret;
> + if (ret < 0)
> + break;
> + }
> + /* The next loop iteration consumes the leftover bytes. */
> + memmove (inbuf, p, inbuf_used);
> + file_offset += read_ret - inbuf_used;
> }
Ok.
> -
> - if (actlen == maxlen)
> - while (1)
> - {
> - ssize_t n;
> - char *new_inbuf;
> -
> - /* Increase the buffer. */
> - new_inbuf = (char *) realloc (inbuf, maxlen + 32768);
> - if (new_inbuf == NULL)
> - {
> - error (0, errno, _("unable to allocate buffer for input"));
> - return -1;
> - }
> - inbuf = new_inbuf;
> - maxlen += 32768;
> - inptr = inbuf + actlen;
> -
> - do
> - {
> - n = read (fd, inptr, maxlen - actlen);
> -
> - if (n == 0)
> - /* No more text to read. */
> - break;
> -
> - if (n == -1)
> - {
> - /* Error while reading. */
> - error (0, errno, _("error while reading the input"));
> - return -1;
> - }
> -
> - inptr += n;
> - actlen += n;
> - }
> - while (actlen < maxlen);
> -
> - if (n == 0)
> - /* Break again so we leave both loops. */
> - break;
> - }
> -
> - /* Now we have all the input in the buffer. Process it in one run. */
> - return process_block (cd, inbuf, actlen);
> + return status;
> }
Ok.
> diff --git a/iconv/tst-iconv_prog-buffer.sh b/iconv/tst-iconv_prog-buffer.sh
> index a9c3729d94..23098ac56a 100644
> --- a/iconv/tst-iconv_prog-buffer.sh
> +++ b/iconv/tst-iconv_prog-buffer.sh
> @@ -50,6 +50,9 @@ echo OUT > "$tmp/out-template"
> : > "$tmp/empty"
> printf '\xff' > "$tmp/0xff"
>
> +# Length should be a prime number, to help with buffer alignment testing.
> +printf '\xc3\xa4\xe2\x80\x94\xe2\x80\x94\xc3\xa4\n' > "$tmp/utf8-sequence"
> +
This is "\xc3\xa4 \xe2\x80\x94 \xe2\x80\x94 \xc3\xa4 \n" - 11 chars with
the newline. Ok.
> +# NB: Extra iconv args are ignored after this point. Actual
> +# multi-byte conversion does not work with tiny buffers.
> +iconv_args="-f UTF-8 -t ASCII"
> +
> +printf 'x\n\xc3' > "$tmp/incomplete"
> +expect_exit 1 run_iconv -o "$tmp/out" "$tmp/incomplete"
> +check_out <<EOF
> +x
> +EOF
Ok.
> +# Test buffering behavior if the buffer ends with an incomplete
> +# multi-byte sequence.
> +prefix=""
> +prefix_length=0
> +while test $prefix_length -lt 12; do
> + echo "info: testing prefix length $prefix_length" 2>&$logfd
> + printf "%s" "$prefix" > "$tmp/prefix"
> + cat "$tmp/prefix" "$tmp/utf8-sequence" > "$tmp/tmp"
> + iconv_args="-f UTF-8 -t UCS-4"
> + run_iconv -o "$tmp/out1" "$tmp/tmp"
> + iconv_args="-f UCS-4 -t UTF-8"
> + run_iconv -o "$tmp/out" "$tmp/out1"
> + expect_files prefix utf8-sequence
> +
> + prefix="$prefix@"
> + prefix_length=$(($prefix_length + 1))
> +done
Ok.
More information about the Libc-alpha
mailing list