chibi-scheme icon indicating copy to clipboard operation
chibi-scheme copied to clipboard

`getgrgid_r` and `getgrnam_r` stubs don't check for non-existent groups

Open Oxyd opened this issue 2 years ago • 1 comments

The relevant definitions in Chibi are: lib/chibi/system.stub:

    (define-c errno getgrgid_r
      (gid_t (result group)
             (link string)
             (value (string-size arg2) int)
             (result pointer group)))

    (define-c errno getgrnam_r
      (string (result group)
              (link string)
              (value (string-size arg2) int)
              (result pointer group)))

lib/chibi/system.sld:

     (define (user-information user)
       (car (if (string? user)
                (getpwnam_r user (make-string 1024))
                (getpwuid_r user (make-string 1024)))))
     (define (group-information group)
       (car (if (string? group)
                (getgrnam_r group (make-string 1024))
                (getgrgid_r group (make-string 1024)))))

man 3 getgrnam says:

On success, getgrnam_r() and getgrgid_r() return zero, and set *result to grp. If no matching group record was found, these functions return 0 and store NULL in *result.

This means that the return value (which is defined to be of type errno in Chibi) doesn't tell the whole story here. Even if these functions return zero, the output parameter may not have been set.

The generated C stub only checks for non-zero return value, however:

sexp sexp_getgrgid_r_stub (sexp ctx, sexp self, sexp_sint_t n, sexp arg0, sexp arg2) {
  int err = 0;
  struct group* tmp1;
  struct group* *tmp4;
  sexp_gc_var3(res, res1, res4);
  if (! sexp_exact_integerp(arg0))
    return sexp_type_exception(ctx, self, SEXP_FIXNUM, arg0);
  if (! sexp_stringp(arg2))
    return sexp_type_exception(ctx, self, SEXP_STRING, arg2);
  sexp_gc_preserve3(ctx, res, res1, res4);
  tmp1 = (struct group*) calloc(1, 1 + sizeof(tmp1[0]));
  tmp4 = (struct group**) calloc(1, 1 + sizeof(tmp4[0]));
  err = getgrgid_r(sexp_uint_value(arg0), tmp1, sexp_string_data(arg2), sexp_string_size(arg2), tmp4);
  if (err) {
  res = SEXP_FALSE;
  } else {
  res1 = sexp_make_cpointer(ctx, sexp_unbox_fixnum(sexp_opcode_arg2_type(self)), tmp1, arg2, 1);
  res4 = sexp_make_cpointer(ctx, sexp_unbox_fixnum(sexp_vector_ref(sexp_opcode_argn_type(self), SEXP_ONE)), tmp4, SEXP_FALSE, 1);
  res = SEXP_NULL;
  sexp_push(ctx, res, res4);
  sexp_push(ctx, res, res1);
  }
  sexp_gc_release3(ctx);
  return res;
}

Together, this all means that calling group-information with an unassigned GID returns bogus values.

This returning of bogus values can be provoked by the tar test: It calls make-tar with GID 502, and there is no guarantee that a group with that GID will exist on the host system.

This was discovered during an IRC discussion with @mnieper . On my system, this bogus result from getgrgid_r means the tar test succeeds with no apparent issue. On @mnieper's system, however, getgrgid_r returned #f, and group-information then attempted to car this #f value, failing the test.

I don't know why it would return #f for him and not me (he's using Ubuntu 22.04 and I'm using Arch Linux, so that probably changes something). But I'm fairly certain that the current Chibi definition of getgrgid_r is broken: It needs to check the value written to the last parameter of the C getgrgid_r in addition to checking the return value.

This also implies that the tar test is broken: It will fail on any system with group 502 missing – at least it will fail once this bug is fixed.

Also, the stub callocing the last parameter is probably undesirable as well. Ideally it would pass &tmp1 and then check whether tmp1 is NULL, as the manpage dictates. Not sure if define-c is sophisticated enough to be able to generate such a stub, though.

Oxyd avatar Jan 29 '23 23:01 Oxyd

Thanks, these need fixing up. Also the tar impl should probably map unknown ids to something reasonable.

ashinn avatar Jan 30 '23 12:01 ashinn

Sorry for the delay. Actually, the tar issue was fixed a while ago but we still needed better handling of the insane getgrgid_r API.

ashinn avatar May 31 '24 10:05 ashinn