Work around compiler bug on assignment from itype with nested `_Nt_array_ptr` to fully checked type

[kyleheadley]

This error appears in our vsftpd benchmarks (-alltypes)

sysdeputil.c:905:35: error: initializing '_Array_ptr<_Nt_array_ptr<char>>' with an expression of incompatible type 'char **'
 _Array_ptr<_Nt_array_ptr<char>> p_env = environ;
                                 ^       ~~~~~~~

It's related to a global variable with an itype inside an unwritable file, and the local copy and usage is inside an #ifdef.

The error needs to be explored more, but this issue us being used as a link target from the benchmark summary that acknowledges the problem.

[mattmccutchen-cci]

The problem may just be that sysdeputil.c isn't seeing the checked declaration for environ. Adding #include <unistd.h> may fix that.

Correction to what I wrote before: Unfortunately, there's no standard header that C programs are expected to include before accessing environ. The POSIX standard recommends that programs hard-code extern char **environ: yuck. That puts the Checked C system headers in a difficult spot. unistd.h may have been the best place to put the declaration given that it is at least mentioned on the same page of the POSIX standard. But if Checked C really wants to solve the problem, ultimately it may have to implicitly include the checked declaration of environ in every translation unit.

[john-h-kastner]

This could be a Checked C compiler bug. environ is in unistd.h with itype(_Nt_array_ptr<_Nt_array_ptr<char>>), but

This is an error:

char ** p : itype(_Nt_array_ptr<_Nt_array_ptr<char>>);
void test(void) {
  _Array_ptr<_Nt_array_ptr<char>> q = p;
}

while this isn't:

char * p : itype(_Nt_array_ptr<char>);
void test(void) {
  _Array_ptr<char> q = p;
}

[mattmccutchen-cci]

Good catch on the compiler bug. I did some more testing, and I'm pretty sure that either the compiler bug or the lack of #include <unistd.h> is sufficient to cause the error. To fix the error, we'll need to address both causes.

In John's examples above, if I put _Checked on the function body, then it uses the checked side of the itype of p and the error goes away. But if I add the following code to sysdeputil.c:

void environ_test(void) _Checked {
  _Array_ptr<_Nt_array_ptr<char>> p_env = environ;
}

then the error still occurs. If I add #include <unistd.h>, then the error in environ_test goes away.

I suspect the compiler bug has to do with the highly technical rules for itypes in section 6.3.8 of the Checked C specification. IIUC, the general idea is that in an unchecked scope, the unchecked side of the itype is used, but the bounds information from the itype is made available to the bounds check. I suspect the problem is that the compiler is keeping only the top-level bounds information, not the bounds information of the inner _Nt_array_ptr.

[mattmccutchen-cci]

Some more thoughts:

Re the itype compiler bug: It looks like a C-style cast works around the problem, just as in microsoft/checkedc-clang#614:

char ** p : itype(_Nt_array_ptr<_Nt_array_ptr<char>>);
void test(void) {
  // Error
  _Array_ptr<_Nt_array_ptr<char>> q = p;
  // No error
  _Array_ptr<_Nt_array_ptr<char>> q_cast = (_Array_ptr<_Nt_array_ptr<char>>) p;
}

So that's probably what 3C should do unless/until the compiler bug is fixed, analogously to #545.

Re having an itype for environ in the first place: I realize that if plain-C code is supposed to hard-code extern char **environ, it's only reasonable to expect that to be changed in some way as part of Checked C porting. The user could just change the hard-coded declaration to extern char **environ : itype(_Nt_array_ptr<_Nt_array_ptr<char>>), but the Checked C documentation should probably recommend including unistd.h instead in case that declaration needs to be updated. There is a risk of unsoundness if a user writes a bogus declaration like extern char **environ: itype(_Nt_array_ptr<_Nt_array_ptr<char>>) count(1000000000), but this falls under the general principle that if a user declares an undefined variable or function with an itype, they are responsible for getting the declaration right (as the warning in the 3c -help text for -infer-types-for-undefs states). So if we make sure that principle is prominent in the Checked C documentation (not just in the 3c -infer-types-for-undefs documentation) and then give the user a hint about what to do about environ specifically, we should be OK. For now, I'll move this to our internal "untriaged issues" list.

The other point here is why 3C is trying to assign environ to an _Array_ptr<_Nt_array_ptr<char>> without a cast of any kind, given that no itype for environ is available in the translation unit. I assume this is because 3C is seeing the extern char **environ : itype(_Nt_array_ptr<_Nt_array_ptr<char>>) from unistd.h in another translation unit. So this is just another case of #507 / #678; I've noted it there.

Now that I've dispatched the sub-issues related to having an itype for environ to other places, I propose that we use this issue to track only a 3C workaround for the compiler bug. I'll update the title accordingly. Kyle, I suppose it would make sense if you link the benchmark error filter entry to both this issue and #507 (or #678) since both will have to be fixed to make the error go away; I'll leave it up to you to develop a syntax for that. :slightly_smiling_face: