Re: [Bug-xorriso] Fixes and improvements for libisoburn

[Thomas Schmitt]

Hi,

please brace for lengthy text. :)

--------------------------------------------------------------------------

Eliska Svobodova wrote:
> 1) Bitwise operations - shift 1 << 31 is used on multiple places through
> xorriso, which is an undefined operation; fix: make 1 unsigned:  1u << 31

Looks good to me. Applied as
  
https://dev.lovelyhq.com/libburnia/libisoburn/commit/fb61be034f3ab95c48e3de5855599c8acf17fa13

--------------------------------------------------------------------------

> 2) Absolute value - you are using abs() to calculate the absolute value of
> r1count and r2count, which are of type long int, so it could be truncated;
> fix: use labs()

They are off_t rather than long int. The whole ado with (double) is
because there is no printf formatter for off_t and the number of "long"s
is pure guesswork on source code level.

So i decided for a (double) based solution, avoiding fabs(3) because
there is no dependency on libm yet.
  
https://dev.lovelyhq.com/libburnia/libisoburn/commit/fb61be034f3ab95c48e3de5855599c8acf17fa13

--------------------------------------------------------------------------

> 3) Dereference of NULL pointers - in xorriso/opts_d_h.c, first_job pointer
> can be in case of failure of Findjob_new a NULL pointer, which is
> dereferenced in ex:

Indeed.


> in xorriso/opts_i_o.c,
> also descr can be NULL, if end_idx <= 0 and execution goes straight to ex:

Yes. But then Xorriso_opt_args() is called with flag bit 8 for disposal
of optv. descr (function parameter "argv") is not dereferenced in this case.

But argv is used for a computation which makes sense only if it is a
valid memory address. To perform it with argv == NULL invites for
misinterpretation, although i deem it unlikely that a malloc() heap
pointer is near enough to 0 to step into that puddle.

So i dediced to add a test for (argv == NUL) in Xorriso_opt_args() in
order to avoid the unlikely mistake which would cause a memory leak.
opts_i_o.c stays as is.


> in xorriso/read_run.c,
> when catcontent is NULL, NULL is put in buf_pt and is passed to write()
> function

This is a tricky situation. A file shall be copied from ISO to hard disk.
Its type is El Torito Boot Catalog which will be mapped to regular file.
What to do if libisofs does not deliver catalog content ?
Create no file ? Create an empty file ?

Currently, the NULL pointer is supposed to be not used in the further
course of the function, because if iso_image_get_bootcat() returns no
catcontent, then catsize is supposed to be 0.
--------------------------
<libisofs/libisofs.h> has:
  * @param content
  *      Will return either NULL or an allocated memory buffer with the
  *      content bytes of the boot catalog.
  *      Dispose it by free() when no longer needed.
  * @param size
  *      Will return the number of bytes in content.
  * @return
  *      1 if reply is valid, 0 if not boot catalog was loaded, < 0 on error.
  *
  * @since 1.1.2
  */
 int iso_image_get_bootcat(IsoImage *image, IsoBoot **catnode, uint32_t *lba,
                           char **content, off_t *size);
---------------------
The implementation in libisofs is a bit complicated. It looks like no
size > 0 can be recorded if content is NULL.
Nevertheless i made sure in iso_image_get_bootcat() that if content is
NULL then size is forced to 0.
  
https://dev.lovelyhq.com/libburnia/libisofs/commit/fe98b35afb2f16b4605d15c523c70541997a72de

So the loop while(todo>0) in libisoburn gets skipped and an empty file
emerges. For now i leave this as is.

Changes applied as
  
https://dev.lovelyhq.com/libburnia/libisoburn/commit/e17db6aa3e80feaed546ad8ebee7a46b01b5d2e1

--------------------------------------------------------------------------

> 4) Missing initialization - in xorriso/iso_manip.c, own_link_stack isn't
> initialized,

Applied as
  
https://dev.lovelyhq.com/libburnia/libisoburn/commit/b35629170077e591af9d32b1e2e95ce5c08160e0

--------------------------------------------------------------------------

> And we have some improvements to make the code safer and cleaner:
>
> 1) Resource leaks - in xorriso/drive_mgt.c, dynamically allocated disc isn't
> freed in case that goto ex; is used; fix: move freeing after ex:

Indeed. Just for completeness i also move the processing of possible
library messages after ex, too.


>                            - in xorriso/emulators.c and
> xorriso/opts_i_o.c,arguments should be freed by Sfile_destroy_argv function

I wonder which tool reported this with what messages.
There is a fourth occasion of the Xorriso_read_lines() loop which is not
in your patch: opts_p_z.c, Xorriso_option_path_list().

Two of the found occasions are false positives. Only those where
Xorriso_read_lines(...,2) is called conditionally are prone to memory
leaks in case of goto ex. I.e. the two in opts_i_o.c are ok by
always calling Xorriso_read_lines(...,2) after the loop.

While analysing the possibilities for a memory leak in
Xorriso_option_path_list() i found a potential leak which happens when
the argv pointer array can be allocated but then its content string cannot.
See the text_io.c part of the changeset.
  
https://dev.lovelyhq.com/libburnia/libisoburn/commit/3eba8cc9210639caa830ed0f1793592bf60b66bf

So how come that the analizer reported two false positives and missed
one true positive ?
I mainly ask because it might be that the reported positives are not
actually about the aspects which we inspected, but about something else.

--------------------------------------------------------------------------

> 2) Overflow in strcpy - in multiple files, there are strcpys which copy
> dynamically allocated strings into static strings, it would be better to use
> strncpy to avoid overflow

This would put a cap on potential memory errors. But an overflow of
a string is also a problem with program logics. So a blanket solution is
not to expect.

In particular:

> --- a/xorriso/drive_mgt.c
> @@ -910,7 +910,8 @@ int Xorriso_toc_line(struct XorrisO *xorriso, int flag)
> - strcpy(xorriso->info_text, xorriso->result_line);

xorriso->result_line is of the same size as xorriso->info_text.
If any oversized text made it into result_line, then the program is
doomed already.

No need for action.


> --- a/xorriso/iso_tree.c
> @@ -865,7 +865,8 @@ int Xorriso_getfattr(struct XorrisO *xorriso,
> -       strcpy(xorriso->result_line, bsl);

(The patch does not address the subsequent sprintf() which would overflow
 if all available space was filled by the strncpy().)

The result size is pre-checked by

     if(strlen(names[i]) + value_lengths[i] >= SfileadrL) {
       sprintf(xorriso->result_line, "# oversized: name %d , value %d bytes\n",
               (int) strlen(names[i]), (int) value_lengths[i]);

Function Sfile_bsl_encoder() cannot inflate the text size by a factor
of 10 to surpass the size of xorriso->result_line.

No need for action.


> --- a/xorriso/misc_funct.c
> @@ -521,7 +521,8 @@ int Decode_xorriso_timestamp(struct tm *erg, char *code,
> - strcpy(buf, code);

The size is pre-checked by

  l= strlen(code);
  if(l>17 || l<10)
    return(0);

No need for action.


> --- a/xorriso/opts_a_c.c
> @@ -368,8 +368,10 @@ int Xorriso_option_application_id(struct XorrisO
> - else
> -   strcpy(xorriso->application_id,name);

The size is pre-checked by

 if(Xorriso_check_name_len(xorriso, name,
                           (int) sizeof(xorriso->application_id),
                           "-application_id", 0) <= 0)
   return(0);

which will complain and refuse if oversize is detected.

No need for action.


> @@ -568,7 +570,8 @@ int Xorriso_option_biblio_file(struct XorrisO *xorriso, 
> char *name, int flag)
>  if(Xorriso_check_name_len(xorriso, name, (int) sizeof(xorriso->biblio_file),
>                            "-biblio_file", 0) <= 0)
> - strcpy(xorriso->biblio_file, name);

The same.

No need for action.


> @@ -847,7 +850,8 @@ treatment_patch:;
> -     strcpy(xorriso->boot_image_bin_path, treatpt + 9);
> @@ -871,7 +876,8 @@ treatment_patch:;
> -     strcpy(xorriso->boot_image_bin_path, treatpt + 9);

Here xorriso should refuse to perform the -boot_image command.

I added length tests with error exit.


> @@ -861,7 +865,8 @@ treatment_patch:;
> -     strcpy(xorriso->boot_image_bin_form, formpt);

This line is only executed with known short texts:

 if(strcmp(formpt, "isolinux")==0 || strcmp(formpt, "grub") == 0)
   isolinux_grub= 1;
 ...
   if(isolinux_grub) {
     ...
     strcpy(xorriso->boot_image_bin_form, formpt);

No need for action.


> @@ -1438,7 +1444,8 @@ int Xorriso_option_cdi(struct XorrisO *xorriso,
> - strcpy(xorriso->wdi, namept);

The length of eff_path was tested in Xorriso_normalize_img_path().
Xorriso_truncate_path_comps() either sets namept to eff_path or it
writes a truncated version of eff_path into path and sets namept to
path.

No need for action.


> @@ -2378,10 +2385,12 @@ int Xorriso_option_concat(struct XorrisO *xorriso,
> -   strcpy(xorriso->list_delimiter, delimiter);

The size is pre-checked by

   ret= Xorriso_check_thing_len(xorriso, argv[*idx + 1],
                                sizeof(xorriso->list_delimiter), "-concat",
                                "Delimiter", 0);
   if(ret <= 0)
     goto ex;

No need for action.


> -   strcpy(xorriso->list_delimiter, delimiter_mem);

delimiter_mem is the memorized content of xorriso->list_delimiter before
the operation began. So it cannot be too large.

No need for action.


> @@ -2427,7 +2436,8 @@ int Xorriso_option_copyright_file(struct XorrisO
> - strcpy(xorriso->copyright_file, name);

The size is pre-checked by

 if(Xorriso_check_name_len(xorriso, name,
                           (int) sizeof(xorriso->copyright_file),
                           "-copyright_file", 0) <= 0)
   return(0);

No need for action.


> --- a/xorriso/opts_p_z.c
> -   strcpy(xorriso->mark_text, xorriso->info_text);

mark_text is indeed smaller than info_text. But info_text only serves
as temporary storage for a string which formerly was in mark_text:

   strcpy(xorriso->info_text, xorriso->mark_text);

No need for action.


> @@ -417,7 +418,8 @@ int Xorriso_option_publisher(struct XorrisO *xorriso,
> strcpy(xorriso->publisher,name);

The size is pre-checked by Xorriso_check_name_len().

No need for action.


> --- a/xorriso/text_io.c
> @@ -612,7 +612,8 @@ klammer_affe:;
> -   strcpy(xorriso->pending_option,cpt);

cpt cannot be longer than SfileadrL-1, because it is a pointer into
a string which was checked for this length:

 ret= Xorriso_dialog_input(xorriso,line, SfileadrL, 1);
 ...
 cpt= line;
 ...

No need for action.

So only one occasion survived:
  
https://dev.lovelyhq.com/libburnia/libisoburn/commit/06346c1e982ee4e813372914b16201cba17ced3f

--------------------------------------------------------------------------

> 3) Memcpy - in libisoburn/isofs_wrap.c, strncpy is used to copy fixed-size
> memory without ending null (Coverity reported it as a mistake), memcpy would
> be probably better in order to express the intent more clearly and to avoid
> false-positive warning in the future.

Maybe it gets smarter and avoids this warning together with all the false
positives above.

The man pages of strncpy(3) and memcpy(3) describe quite similar semantics
with the difference that strncpy() would be safe against reading
inappropriate bytes if src is shorter than n.
But on the other hand, a manipulation of an ISO 9660 Primary Volume
Descriptor is sypposed to be a binary data operation. So memset() is
indeed more appropriate than strncpy().

Applied by
  
https://dev.lovelyhq.com/libburnia/libisoburn/commit/fe5fd8eefa42273c6d84d240582eee5c90412b43

--------------------------------------------------------------------------

I hope i got everything properly processed now. Review is welcome.


Have a nice day :)

Thomas