kern/53831: libhfs might be wrong in edge cases

[coypu%sdf.org@localhost]

>Number:         53831
>Category:       kern
>Synopsis:       libhfs might be wrong in edge cases
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Thu Jan 03 17:25:00 +0000 2019
>Originator:     coypu
>Release:        NetBSD 8.99.27
>Organization:
>Environment:
NetBSD planets 8.99.27 NetBSD 8.99.27 (GENERIC) #1: Fri Dec 21 09:30:22 IST 2018  fly@planets:/home/fly/obj/sys/arch/amd64/compile/GENERIC amd64
>Description:
Foreword:

Looking at the code,

HFS_LIBERR is (if you're not PCC, which will print stack garbage)



     65 #define HFS_LIBERR(format, ...) \
     66 	do{ hfslib_error(format, __FILE__, __LINE__, ##__VA_ARGS__); \
     67 		goto error; } while(/*CONSTCOND*/ 0)

In many places, the error case looks like this:

      result = 1;

      if (!test_for_things)
           HFS_LIBERR("something");

error:
       cleanup...
       return result;

For this to function in a sensible way, result must only be set to zero towards the end, not in the middle.




Unfortunately, hfslib_open_volume:


    176 	result = 1;
...

    336 	/*
    337 	 * If this volume uses case-folding comparison and the folding table hasn't
    338 	 * been created yet, do that here. (We don't do this in hfslib_init()
    339 	 * because the table is large and we might never even need to use it.)
    340 	 */
    341 	if (out_vol->keycmp == hfslib_compare_catalog_keys_cf && hfs_gcft == NULL)
    342 		result = hfslib_create_casefolding_table();
    343 	else
    344 		result = 0;
    345 
    346 	/*
    347 	 * Find and store the volume name.
    348 	 */
    349 	if (hfslib_make_catalog_key(HFS_CNID_ROOT_FOLDER, 0, NULL, &rootkey) == 0)
    350 		HFS_LIBERR("could not make root search key");
...

    359 error:
    360 	if (result != 0 && isopen)
    361 		hfslib_close_volume(out_vol, cbargs);
    362 	if (buffer != NULL)
    363 		hfslib_free(buffer, cbargs);
    364 	return result;



Further HFS_LIBERR might be misbehaving, looking like success but failing.
I am not sure this is the case yet, but bringing this to attention.

It might be better practice to have HFS_LIBERR set result to a non-zero value too.
>How-To-Repeat:

>Fix: