[libbluray-devel] [PATCH][libudfread] Cleanup some conversion/compare warnings.

[Petri Hintukainen]

ma, 2017-05-29 kello 12:09 +0200, ace at kodi.tv kirjoitti:
> From: ace20022 <ace20022 at ymail.com>
> 
> ---
>  src/udfread.c | 32 +++++++++++++++++++++++---------
>  1 file changed, 23 insertions(+), 9 deletions(-)
> 
> diff --git a/src/udfread.c b/src/udfread.c
> index bffb9ea..3662e20 100644
> --- a/src/udfread.c
> +++ b/src/udfread.c
> @@ -139,7 +139,7 @@ static void *_safe_realloc(void *p, size_t s)
>  #define utf16lo_to_utf8(out, out_pos, out_size, ch) \
>    do {                                              \
>      if (ch < 0x80) {                                \
> -      out[out_pos++] = ch;                          \
> +      out[out_pos++] = (uint8_t)ch;                          \
>      } else {                                        \
>        out_size++;                                   \
>        out = (uint8_t *)_safe_realloc(out, out_size);\
> 

Applied

> @@ -237,6 +237,9 @@ static uint32_t _read_blocks(udfread_block_input
> *input,
>          return 0;
>      }
>  
> +    if (((int64_t)lba + (int64_t)nblocks) > UINT32_MAX)
> +      return 0;
> +
>      result = input->read(input, lba, buf, nblocks, flags);
>  
>      return result < 0 ? 0 : result;

Doesn't match patch description. Does this fix something ?
Maybe it should be handled in input->read() or in the source of
incorrect values ?

> @@ -943,11 +946,17 @@ static struct udf_dir *_read_dir(udfread *udf,
> const struct long_ad *icb)
>          free_file_entry(&fe);
>          return NULL;
>      }
> +    // ECMA ECMA 167 4/8.6.2
> +    if (fe->length > UINT32_MAX) {
> +      udf_error("maximum directory size exceeded\n");
> +      free_file_entry(&fe);
> +      return NULL;
> +    }
>
>      if (fe->content_inline) {
>          dir = (struct udf_dir *)calloc(1, sizeof(struct udf_dir));
>          if (dir) {
> -            if (_parse_dir(&fe->data.content[0], fe->length, dir) <
> 0) {
> +            if (_parse_dir(&fe->data.content[0], (uint32_t)fe-
> >length, dir) < 0) {
>                  udf_error("failed parsing inline directory file\n");
>                  _free_dir(&dir);
>              }

I think using fe->length here is wrong. It should use recorded
information length instead. This should also be fixed when reading such
files in udfread_file_read().

> @@ -1479,12 +1488,16 @@ uint32_t udfread_read_blocks(UDFFILE *p, void
> *buf, uint32_t file_block, uint32_ 
>  static ssize_t _read(UDFFILE *p, void *buf, size_t bytes)
>  {
> -    /* start from middle of block ? */
> +    /* start from middle of block ?
> +     * maximal file size, i.e. position, is 2^32 * block size
> +     */
> +
>      size_t pos_off = p->pos % UDF_BLOCK_SIZE;
> +    uint32_t file_block = (uint32_t)(p->pos / UDF_BLOCK_SIZE);
>      if (pos_off) {
>          size_t chunk_size = UDF_BLOCK_SIZE - pos_off;
>          if (!p->block_valid) {
> -            if (udfread_read_blocks(p, p->block, p->pos /
> UDF_BLOCK_SIZE, 1, 0) != 1) {
> +            if (udfread_read_blocks(p, p->block, file_block, 1, 0)
> != 1) {
>                  return -1;
>              }
>              p->block_valid = 1;
> @@ -1500,7 +1513,7 @@ static ssize_t _read(UDFFILE *p, void *buf,
> size_t bytes)
>      /* read full block(s) ? */
>      if (bytes >= UDF_BLOCK_SIZE) {
>          uint32_t num_blocks = bytes / UDF_BLOCK_SIZE;
> -        num_blocks = udfread_read_blocks(p, buf, p->pos /
> UDF_BLOCK_SIZE, num_blocks, 0);
> +        num_blocks = udfread_read_blocks(p, buf, file_block,
> num_blocks, 0);
>          if (num_blocks < 1) {
>              return -1;
>          }
> @@ -1509,7 +1522,7 @@ static ssize_t _read(UDFFILE *p, void *buf,
> size_t bytes)
>      }
>  
>      /* read beginning of a block */
> -    if (udfread_read_blocks(p, p->block, p->pos / UDF_BLOCK_SIZE, 1,
> 0) != 1) {
> +    if (udfread_read_blocks(p, p->block, file_block, 1, 0) != 1) {
>          return -1;
>      }
>      p->block_valid = 1;
> 

Applied

> @@ -1524,9 +1537,10 @@ static ssize_t _read(UDFFILE *p, void *buf,
> size_t bytes)
>  ssize_t udfread_file_read(UDFFILE *p, void *buf, size_t bytes)
>  {
>      uint8_t *bufpt = (uint8_t *)buf;
> +    int64_t file_size = udfread_file_size(p);

As file size can't really be < 0, maybe it should be stored in unsigned
here.
 
>      /* sanity checks */
> -    if (!p || !buf || p->pos < 0) {
> +    if (!p || !buf || p->pos < 0 || p->pos > file_size || file_size
> < 0) {
>          return -1;
>      }
>      if ((ssize_t)bytes < 0 || (int64_t)bytes < 0) {
> 

Doesn't match patch description.

I'm not sure if p->pos can be > file_size here. If it can, the check is
useful. Maybe it should return 0 (EOF) instead of an error ?

I think the last check is redundant; we already know
  pos >= 0 && pos <= file_size
  => 0 <= pos <= file_size
  => 0 <= file_size

> @@ -1534,8 +1548,8 @@ ssize_t udfread_file_read(UDFFILE *p, void
> *buf, size_t bytes)
>      }
>  
>      /* limit range to file size */
> -    if ((uint64_t)p->pos + bytes > (uint64_t)udfread_file_size(p)) {
> -        bytes = udfread_file_size(p) - p->pos;
> +    if (p->pos + (uint64_t)bytes >(uint64_t)file_size) {
> +        bytes = (size_t)(file_size - p->pos);
>      }
>  
>      /* small files may be stored inline in file entry */