[LibOS,PAL] Introduce etc emulation (currently only 'resolv.conf')
#889
Conversation
There was a problem hiding this comment.
Reviewed 7 of 19 files at r1, all commit messages.
Reviewable status: 7 of 19 files reviewed, 21 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @oshogbo)
a discussion (no related file):
The PR description is wrong? Why does it talk about the hostname?
a discussion (no related file):
On a range of my local machines, I checked /etc/resolv.conf. The range of machines included:
- Docker containers
- My local dev machine with Ubuntu 20.04
- My local perf machine with CentOS 8
- My private Azure VM
I did not find sortlist keyword in any of the files. So unless others find it on their deployments, it seems fine to ignore this keyword.
However, I found the following options keywords on my Azure VM:
dimakuv@accvm:~$ cat /etc/resolv.conf
nameserver 127.0.0.53
options edns0 trust-ad # note this!
search anxjiuxym1nefegvxud0s5rhfb.bx.internal.cloudapp.netI haven't checked this PR yet, but I would do the following with the options:
edns0-- recognize it and propagate into Graminetrust-ad-- recognize it but do not propagate into Gramine
Documentation/manifest-syntax.rst line 158 at r1 (raw file):
This specifies whether to emulate runtime files under ``/etc``. In the case of Linux-SGX PAL, this is achieved by taking the host's ``/etc`` files, filter supported options, and sanitize them.
And in case of other PALs (e.g. Linux)? This description is ambigious.
I think we need to write something like this:
This is achieved by taking the host's ``/etc`` files, parsing them, and re-creating them
inside Gramine's file system. For security-enforcing modes (such as SGX), Gramine
additionally sanitizes the contents of these files, failing if any of the files is
incorrectly formatted.
Note that Gramine's parsers for these files support only a subset of the corresponding
specifications. See below for the list of supported keywords/formats for each file.
The set of extra runtime files ...
Documentation/manifest-syntax.rst line 158 at r1 (raw file):
This specifies whether to emulate runtime files under ``/etc``. In the case of Linux-SGX PAL, this is achieved by taking the host's ``/etc`` files, filter supported options, and sanitize them.
What do you mean by filter supported options? Does Gramine fail if it finds an unsupported option, or simply removes it from the in-Gramine representation?
Documentation/manifest-syntax.rst line 166 at r1 (raw file):
- ``nameserver`` - ``search`` - ``options`` (``inet6`` | ``rotate``)
I feel like ndots:n may be important to parse & propagate to Gramine?
Documentation/manifest-syntax.rst line 166 at r1 (raw file):
- ``nameserver`` - ``search`` - ``options`` (``inet6`` | ``rotate``)
I see no problem in supporting use-vc option, but not blocking
libos/src/fs/etc/fs.c line 15 at r1 (raw file):
#include "libos_fs_pseudo.h" static int put_string(char** buf, ssize_t* bufsize, const char* fmt, ...) {
Why ssize_t? Simply size_t
libos/src/fs/etc/fs.c line 21 at r1 (raw file):
va_end(ap); if (ret < 0) return ret;
I think you should also fail when truncation happened:
if (ret >= *bufsize)
return -EOVERFLOW;
And you can remove the below assert.
libos/src/fs/etc/fs.c line 26 at r1 (raw file):
*buf += ret; return ret;
Please simply return 0.
libos/src/fs/etc/fs.c line 29 at r1 (raw file):
} static int provide_etc_resolv(struct libos_dentry* dent, char** out_data, size_t* out_size) {
I'd rename to provide_etc_resolv_conf, so it is clear to what file it refers to
libos/src/fs/etc/fs.c line 32 at r1 (raw file):
__UNUSED(dent); ssize_t size = 0;
Why ssize_t? Simply size_t
libos/src/fs/etc/fs.c line 39 at r1 (raw file):
/* search - lets assume maximum length of entries, plus a new line and white spaces */ size += strlen("search") + 1; size += g_pal_public_state->dns_host.dnsrch_count * (PAL_HOSTNAME_MAX + 1);
I assume that + 1 here is for the whitespace between local domain names?
libos/src/fs/etc/fs.c line 39 at r1 (raw file):
/* search - lets assume maximum length of entries, plus a new line and white spaces */ size += strlen("search") + 1; size += g_pal_public_state->dns_host.dnsrch_count * (PAL_HOSTNAME_MAX + 1);
Shouldn't you size += 1; for the newline of the search ... line?
libos/src/fs/etc/fs.c line 41 at r1 (raw file):
size += g_pal_public_state->dns_host.dnsrch_count * (PAL_HOSTNAME_MAX + 1); /* and lets add some space for each option */ size += (g_pal_public_state->dns_host.inet6 ? 32 : 0) +
Shouldn't you also add size += strlen("options ") ;
libos/src/fs/etc/fs.c line 60 at r1 (raw file):
(addr & 0x0000FF00) >> 8, (addr & 0x000000FF)); } else { uint16_t *addrv6 = g_pal_public_state->dns_host.nsaddr_list[i].ipv6;
uint16_t* (wrong alignment of *)
libos/src/fs/etc/fs.c line 83 at r1 (raw file):
} if (g_pal_public_state->dns_host.inet6) { ret = put_string(&ptr, &size, "options inet6\n");
I don't think this is correct formatting. At least that's not what I observed when googling and checking my machines. I always saw this format:
options option1 option2 ...
and I've never seen your format:
options option1
options option2
...
I think we should change to the "expected" format, even if the spec allows your current format.
libos/src/fs/etc/fs.c line 93 at r1 (raw file):
} assert(size >= 0);
This won't be needed.
libos/src/fs/etc/fs.c line 97 at r1 (raw file):
size_t finalsize = strlen(data); char* finalbuf = malloc(finalsize); if (finalbuf == NULL) {
Just !finalbuf
libos/src/fs/etc/fs.c line 100 at r1 (raw file):
ret = -ENOMEM; goto out; }
Please add an assert(finalsize <= size);
libos/src/fs/etc/fs.c line 113 at r1 (raw file):
int init_etcfs(void) { pseudo_add_str(NULL, "emulate-etc-resolv", &provide_etc_resolv);
I'd rename to emulate-etc-resolv-conf, so it is clear to what file it refers to
pal/include/host/linux-common/etc_host_info.h line 1 at r1 (raw file):
/* SPDX-License-Identifier: LGPL-3.0-or-later */
TODO(dimakuv): continue reviewing from here
There was a problem hiding this comment.
Reviewable status: 5 of 19 files reviewed, 21 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @oshogbo)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
The PR description is wrong? Why does it talk about the hostname?
Yes, my bad.
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
On a range of my local machines, I checked
/etc/resolv.conf. The range of machines included:
- Docker containers
- My local dev machine with Ubuntu 20.04
- My local perf machine with CentOS 8
- My private Azure VM
I did not find
sortlistkeyword in any of the files. So unless others find it on their deployments, it seems fine to ignore this keyword.However, I found the following
optionskeywords on my Azure VM:dimakuv@accvm:~$ cat /etc/resolv.conf nameserver 127.0.0.53 options edns0 trust-ad # note this! search anxjiuxym1nefegvxud0s5rhfb.bx.internal.cloudapp.netI haven't checked this PR yet, but I would do the following with the options:
edns0-- recognize it and propagate into Graminetrust-ad-- recognize it but do not propagate into Gramine
So I proposed more extensive list of it, but Borys suggested in issue to limit it to minimum (#689).
Do you think it's worth implementing so many options initially? I would say most common ones (name server and search) would be enough for start? (disclaimer: I'm no sysadmin)
Adding new options is very simple so not sure what to do.
So maybe for now lets not discuss in this PR which options should or should not be supported, because there are very easy to add and lets focus on the reset.
Documentation/manifest-syntax.rst line 158 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
What do you mean by
filter supported options? Does Gramine fail if it finds an unsupported option, or simply removes it from the in-Gramine representation?
Just ignores it.
Documentation/manifest-syntax.rst line 158 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
And in case of other PALs (e.g. Linux)? This description is ambigious.
I think we need to write something like this:
This is achieved by taking the host's ``/etc`` files, parsing them, and re-creating them inside Gramine's file system. For security-enforcing modes (such as SGX), Gramine additionally sanitizes the contents of these files, failing if any of the files is incorrectly formatted. Note that Gramine's parsers for these files support only a subset of the corresponding specifications. See below for the list of supported keywords/formats for each file. The set of extra runtime files ...
We don't really fails, if the files are wrongly formated, we will just print errors/warnings.
Documentation/manifest-syntax.rst line 166 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I feel like
ndots:nmay be important to parse & propagate to Gramine?
Ditto.
Documentation/manifest-syntax.rst line 166 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I see no problem in supporting
use-vcoption, but not blocking
Ditto.
libos/src/fs/etc/fs.c line 15 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why
ssize_t? Simplysize_t
Done.
libos/src/fs/etc/fs.c line 21 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I think you should also fail when truncation happened:
if (ret >= *bufsize) return -EOVERFLOW;And you can remove the below assert.
Can it happend?
This is why we overestimate the size of buffer, and try just the assert it.
libos/src/fs/etc/fs.c line 26 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please simply
return 0.
Done.
libos/src/fs/etc/fs.c line 29 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I'd rename to
provide_etc_resolv_conf, so it is clear to what file it refers to
Done.
libos/src/fs/etc/fs.c line 32 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why
ssize_t? Simplysize_t
Done.
libos/src/fs/etc/fs.c line 39 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I assume that
+ 1here is for the whitespace between local domain names?
Between local domains and between search and first local domain.
We count the prefix space not suffix space.
libos/src/fs/etc/fs.c line 39 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Shouldn't you
size += 1;for the newline of thesearch ...line?
It is ther:
size += strlen("search") + 1; // search + new_line
dns_host.dnsrch_count * (PAL_HOSTNAME_MAX + 1) -> size of hostname + 1 whitespace
libos/src/fs/etc/fs.c line 41 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Shouldn't you also add
size += strlen("options ") ;
32 is covering all of it here, but I will do this more explicate maybe.
libos/src/fs/etc/fs.c line 60 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
uint16_t*(wrong alignment of*)
Done.
libos/src/fs/etc/fs.c line 83 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I don't think this is correct formatting. At least that's not what I observed when googling and checking my machines. I always saw this format:
options option1 option2 ...and I've never seen your format:
options option1 options option2 ...I think we should change to the "expected" format, even if the spec allows your current format.
This is correct formatting, and I think it is easier to generate it.
But If you really want I can change it.
libos/src/fs/etc/fs.c line 93 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This won't be needed.
Done.
libos/src/fs/etc/fs.c line 97 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Just
!finalbuf
Done.
libos/src/fs/etc/fs.c line 100 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please add an
assert(finalsize <= size);
Done.
libos/src/fs/etc/fs.c line 113 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I'd rename to
emulate-etc-resolv-conf, so it is clear to what file it refers to
Done.
There was a problem hiding this comment.
Reviewed 12 of 19 files at r1.
Reviewable status: 17 of 19 files reviewed, 73 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @oshogbo)
pal/include/host/linux-common/etc_host_info.h line 1 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
TODO(dimakuv): continue reviewing from here
OK, continuing my review.
pal/include/pal/pal.h line 37 at r1 (raw file):
#define PAL_HOSTNAME_MAX 255 /* DNS limits */
maybe, ..., used in resolv.conf emulation
pal/include/pal/pal.h line 38 at r1 (raw file):
/* DNS limits */ #define PAL_MAXNS 3
I would add a comment that this is the max number of namespace servers
pal/include/pal/pal.h line 39 at r1 (raw file):
/* DNS limits */ #define PAL_MAXNS 3 #define PAL_MAXDNSRCH 6
I would add a comment that this is the max number of search local domain names
pal/include/pal/pal.h line 107 at r1 (raw file):
}; struct pal_dns_host_conf {
I would add a top comment here /* Used in resolv.conf emulation */
pal/src/host/linux/pal_main.c line 130 at r1 (raw file):
if (get_resolv_conf(&g_pal_public_state.dns_host) < 0) { INIT_FAIL("Unable to get resolv.conf");
I would prefer the full name /etc/resolv.conf
Also, maybe get -> parse?
pal/src/host/linux/pal_main.c line 423 at r1 (raw file):
ret = toml_bool_in(g_pal_public_state.manifest_root, "libos.emulate_etc_files", false, &g_pal_public_state.emulate_etc_files);
Please align to the beginning of the first arg.
pal/src/host/linux/pal_main.c line 428 at r1 (raw file):
} /* Get host information only for the first process. This information will be
Get host information -> Get host /etc information
pal/src/host/linux-common/etc_host_info.c line 7 at r1 (raw file):
/* * This file contains the APIs to expose host information.
I would add smth like this:
/*
* This file contains the APIs to expose host information:
* - parses host file `/etc/resolv.conf` into `struct pal_dns_host_conf`
And in follow up PRs (if any), add more items to this list
pal/src/host/linux-common/etc_host_info.c line 10 at r1 (raw file):
*/ #include <asm/errno.h>
Do you even use this header?
pal/src/host/linux-common/etc_host_info.c line 16 at r1 (raw file):
#include "linux_utils.h" #define PAL_MAX_HOSTNAME 255
You should remove this one and use PAL_HOSTNAME_MAX
pal/src/host/linux-common/etc_host_info.c line 40 at r1 (raw file):
} static bool parse_ip_addr_ipv4(struct pal_dns_host_conf *conf, const char** pptr) {
*conf -- wrong alignment
pal/src/host/linux-common/etc_host_info.c line 40 at r1 (raw file):
} static bool parse_ip_addr_ipv4(struct pal_dns_host_conf *conf, const char** pptr) {
It would be better to have static bool parse_ip_addr_ipv4(const char* ptr, uint32_t* addr), and move the logic of updating the DNS info and buffer-pointer into the caller
pal/src/host/linux-common/etc_host_info.c line 41 at r1 (raw file):
static bool parse_ip_addr_ipv4(struct pal_dns_host_conf *conf, const char** pptr) { long octed;
What is this word octed? What's it supposed to mean?
pal/src/host/linux-common/etc_host_info.c line 45 at r1 (raw file):
char* next; uint32_t addr = 0; int i;
Move i to under the for loop
pal/src/host/linux-common/etc_host_info.c line 50 at r1 (raw file):
octed = strtol(ptr, &next, 10); if (ptr == next) return false;
If we return false, we don't update *pptr. Doesn't it mean that we'll get stuck forever?
Same for all other such places.
pal/src/host/linux-common/etc_host_info.c line 66 at r1 (raw file):
return false; if (i == 1) { /* Address X.Y has to be converted to: X.0.0.Y */
Where did you get these rules?
pal/src/host/linux-common/etc_host_info.c line 69 at r1 (raw file):
addr = ((addr & 0xFF00) << 16) | (addr & 0xFF); } else if (i == 2) { /* Address X.Y.Z has to be converted to: X.0.Y.Z */
This doesn't correspond to the actual logic? The logic seems to do X.Y.0.Z
pal/src/host/linux-common/etc_host_info.c line 71 at r1 (raw file):
/* Address X.Y.Z has to be converted to: X.0.Y.Z */ addr = ((addr & 0xFFFF00) << 8) | (addr & 0xFF); }
What if we have i == 0? Even if it's Address X has to be converted to: 0.0.0.X, I think it should be mentioned somewhere in comments
pal/src/host/linux-common/etc_host_info.c line 73 at r1 (raw file):
} conf->nsaddr_list[conf->nsaddr_list_count].is_ipv6 = false;
Can we add an assert(conf->nsaddr_list_count < PAL_MAXNS)
pal/src/host/linux-common/etc_host_info.c line 84 at r1 (raw file):
* Supported notations: * Full IPv6 address * Abbreviations `::1`, `ff::1:2`
Please make it a list:
* Supported notations:
* - Full IPv6 address
* - Abbreviations `::1`, `ff::1:2`
pal/src/host/linux-common/etc_host_info.c line 86 at r1 (raw file):
* Abbreviations `::1`, `ff::1:2` */ static bool parse_ip_addr_ipv6(struct pal_dns_host_conf* conf, const char** pptr) {
It would be better to have static bool parse_ip_addr_ipv6(const char* ptr, uint16_t* addr), and move the logic of updating the DNS info and buffer-pointer into the caller
pal/src/host/linux-common/etc_host_info.c line 90 at r1 (raw file):
const char* ptr = *pptr; char* next; int i, twocomas;
Please move to the first usages
pal/src/host/linux-common/etc_host_info.c line 102 at r1 (raw file):
if (i == 0) { if (*(ptr + 1) != ':') return false;
Why is this so? Why do we fail here?
pal/src/host/linux-common/etc_host_info.c line 106 at r1 (raw file):
} twocomas = i; i--;
I'm lost, I don't understand how this parsing works. Could you please explain in the comments a bit?
Can i become negative here?
pal/src/host/linux-common/etc_host_info.c line 130 at r1 (raw file):
if (twocomas == -1 && i != 7) return false; if (twocomas != -1 && i >= 7)
i can be greater than 7?
pal/src/host/linux-common/etc_host_info.c line 139 at r1 (raw file):
} conf->nsaddr_list[conf->nsaddr_list_count].is_ipv6 = true;
Can we add an assert(conf->nsaddr_list_count < PAL_MAXNS)
pal/src/host/linux-common/etc_host_info.c line 152 at r1 (raw file):
if (conf->nsaddr_list_count >= PAL_MAXNS) { log_warning("Host's resolv.conf contains more then %d nameservers, skipping", PAL_MAXNS);
I'm pretty sure Gramine hasn't yet loaded loader.log_level manifest option, so the current log level is anyway ERROR. Which means that this WARNING won't be printed at all, never.
Please verify my assumption (e.g., in a Docker container with modified /etc/resolv.conf), but I'm pretty sure.
So, we should use log_error() in this file, everywhere.
UPDATE: Actually, I think Linux-SGX PAL should work fine, but Linux PAL probably not...
pal/src/host/linux-common/etc_host_info.c line 152 at r1 (raw file):
if (conf->nsaddr_list_count >= PAL_MAXNS) { log_warning("Host's resolv.conf contains more then %d nameservers, skipping", PAL_MAXNS);
then -> than
pal/src/host/linux-common/etc_host_info.c line 157 at r1 (raw file):
/* * Check if nameserver os using IPv6 or IPv4.
os -> is
pal/src/host/linux-common/etc_host_info.c line 159 at r1 (raw file):
* Check if nameserver os using IPv6 or IPv4. * If address contain ':', it is a IPv6 address. * If address contain '.', it is a IPv4 address.
contains (2x)
pal/src/host/linux-common/etc_host_info.c line 163 at r1 (raw file):
while (!is_end_of_word(*ptr)) { if (*ptr == ':') { ipv6 = true;
Shouldn't you also break here? It's obvious at this point that this is IPv6
pal/src/host/linux-common/etc_host_info.c line 247 at r1 (raw file):
static struct { const char* keyword; void (*set_option)(struct pal_dns_host_conf* conf, const char** pptr);
set_value would be a better name...
pal/src/host/linux-common/etc_host_info.c line 259 at r1 (raw file):
const char* startline = buf; while (*ptr != 0x00) {
You use \0 in other places, please also use in this function
pal/src/host/linux-common/etc_host_info.c line 264 at r1 (raw file):
startline = ptr; continue; } else if (*startline == *ptr && *ptr == '#') {
? Did you mean startline == ptr (without asterisks)?
pal/src/host/linux-common/etc_host_info.c line 270 at r1 (raw file):
} else if ((ptr != startline) && (*ptr == ' ' || *ptr == '\t')) { for (size_t i = 0; resolv_keys[i].keyword != NULL; i++) { if (strncmp(startline, resolv_keys[i].keyword, ptr - startline - 1) ==
Don't you have a buffer overread if ptr is somewhere on a very long word (e.g. ptr points to superlongword ). In this case we overread resolv_keys[i].keyword...
I think you should check the strlen(resolv_keys[i].keyword) first.
pal/src/host/linux-common/etc_host_info.c line 271 at r1 (raw file):
for (size_t i = 0; resolv_keys[i].keyword != NULL; i++) { if (strncmp(startline, resolv_keys[i].keyword, ptr - startline - 1) == 0) {
We have a char limit of 100-per-line, but you seem to have smth like 90?
pal/src/host/linux-common/etc_host_info.c line 274 at r1 (raw file):
/* Because the buffer in strncmp is not ended with 0x00, lets * verify that the length of keywords are the same. */ if (resolv_keys[i].keyword[ptr - startline] != 0x00)
This won't be needed, see my other comment
pal/src/host/linux-common/etc_host_info.c line 296 at r1 (raw file):
} parse_resolv_conf(conf, buf);
Why don't we error out on parsing errors? We currently may have a weird case when /etc/resolv.conf was garbled, and Gramine exposes /etc/resolv.conf that is empty... Is this our intention?
pal/src/host/linux-common/etc_host_info.c line 180 at r2 (raw file):
} static void parse_list(struct pal_dns_host_conf* conf, const char** pptr,
Maybe parse_values_list_in_one_line()?
pal/src/host/linux-common/etc_host_info.c line 201 at r2 (raw file):
static void resolv_search_setter(struct pal_dns_host_conf* conf, const char* ptr, size_t length) { if (length >= PAL_MAX_HOSTNAME) { log_warning("One of the search domains in host's resolv.conf is to long (larger then %d), "
then -> than
pal/src/host/linux-common/etc_host_info.c line 201 at r2 (raw file):
static void resolv_search_setter(struct pal_dns_host_conf* conf, const char* ptr, size_t length) { if (length >= PAL_MAX_HOSTNAME) { log_warning("One of the search domains in host's resolv.conf is to long (larger then %d), "
to long -> too long
pal/src/host/linux-common/etc_host_info.c line 201 at r2 (raw file):
static void resolv_search_setter(struct pal_dns_host_conf* conf, const char* ptr, size_t length) { if (length >= PAL_MAX_HOSTNAME) { log_warning("One of the search domains in host's resolv.conf is to long (larger then %d), "
Actually, can you print the domain name here?
pal/src/host/linux-sgx/host_ecalls.h line 10 at r1 (raw file):
int ecall_enclave_start(char* libpal_uri, char* args, size_t args_size, char* env, size_t env_size, int parent_stream_fd, sgx_target_info_t* qe_targetinfo, struct pal_host_info* host_info);
Didn't @boryspoplawski want this to be in two arguments -- one for topo_info and another for etc_host_info?
pal/src/host/linux-sgx/host_main.c line 632 at r1 (raw file):
/* Parses only the information needed by the untrusted PAL to correctly initialize the enclave. */ static int parse_loader_config(char* manifest, struct pal_enclave* enclave_info, bool *emulate_etc) {
Wrong alignment of *
pal/src/host/linux-sgx/host_main.c line 632 at r1 (raw file):
/* Parses only the information needed by the untrusted PAL to correctly initialize the enclave. */ static int parse_loader_config(char* manifest, struct pal_enclave* enclave_info, bool *emulate_etc) {
Please call it out_emulate_etc
pal/src/host/linux-sgx/pal_ecall_types.h line 23 at r1 (raw file):
struct pal_topo_info topo_info; struct pal_dns_host_conf dns_host; };
I think it belongs to pal_host.h
But also, I would just split this into two args, instead of adding everything in this one arg. This is also what @boryspoplawski wanted.
pal/src/host/linux-sgx/pal_linux.h line 25 at r1 (raw file):
#include "log.h" #include "pal.h" #include "pal_ecall_types.h"
This won't be needed, see my other comment.
pal/src/host/linux-sgx/pal_main.c line 301 at r1 (raw file):
} static int init_emulation_etc_files(struct pal_host_info* shallow_host_info) {
There is no need in shallow_ because you don't access any nested objects (no nested pointers) in this struct with regards to /etc/ info.
pal/src/host/linux-sgx/pal_main.c line 303 at r1 (raw file):
static int init_emulation_etc_files(struct pal_host_info* shallow_host_info) { struct pal_dns_host_conf *pub_dns = &g_pal_public_state.dns_host; struct pal_dns_host_conf *shallow_dns = &shallow_host_info->dns_host;
Why do you even need these helper variables? Just use normal variables. Yes, the names are longer, but much easier to read the code then.
pal/src/host/linux-sgx/pal_main.c line 312 at r1 (raw file):
log_error("To many nameservers provided"); return -EINVAL; }
Please add empty line after this one
pal/src/host/linux-sgx/pal_main.c line 316 at r1 (raw file):
for (i = 0; i < pub_dns->nsaddr_list_count; i++) { coerce_untrusted_bool(&shallow_dns->nsaddr_list[i].is_ipv6); /* All binnary IP address are already valid ones. */
binary
addresses
pal/src/host/linux-sgx/pal_main.c line 334 at r1 (raw file):
for (i = 0, j = 0; i < shallow_dns->dnsrch_count; i++) { if (!is_hostname_valid(shallow_dns->dnsrch[i])) { log_warning("One of host's search entries is invalid, skipping it");
Actually, I would print the name here.
pal/src/host/linux-sgx/pal_main.c line 680 at r1 (raw file):
log_error("Unable to read host info"); ocall_exit(1, /*is_exitgroup=*/true); }
Yep, I definitely dislike that you rearranged a lot of code here, in Linux-SGX PAL's main func. Let's not move topo_info existing code around, but rather add a new argument for etc_info and new code.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 58 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @oshogbo)
a discussion (no related file):
Previously, oshogbo (Mariusz Zaborski) wrote…
So I proposed more extensive list of it, but Borys suggested in issue to limit it to minimum (#689).
Do you think it's worth implementing so many options initially? I would say most common ones (name server and search) would be enough for start? (disclaimer: I'm no sysadmin)Adding new options is very simple so not sure what to do.
So maybe for now lets not discuss in this PR which options should or should not be supported, because there are very easy to add and lets focus on the reset.
OK. I'm fine with this.
I think we'll need to create follow-up PRs with the following additional keywords immediately:
options edns0-- propagate into Gramineoptions trust-ad-- ignore with some clear log messagendots:n-- propagate into Gramineuse-vc-- propagate into Gramine
Documentation/manifest-syntax.rst line 161 at r2 (raw file):
Note that Gramine's parsers for these files support only a subset of the corresponding specifications. See below for the list of supported keywords/formats for each file.
Please re-wrap to 80 chars per limit (we use 80 for documentation, 100 for code)
Documentation/manifest-syntax.rst line 161 at r2 (raw file):
Note that Gramine's parsers for these files support only a subset of the corresponding specifications. See below for the list of supported keywords/formats for each file.
Please add a sentence that all other keywords/formats are ignored, with warning messages.
libos/src/fs/etc/fs.c line 21 at r1 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
Can it happend?
This is why we overestimate the size of buffer, and try just the assert it.
I mean, it can if there is a very-very long option name that doesn't fit into 32 chars. This overestimation is not precise and may not cover all cases in the real world, and we end up with buffer overflows in production mode of Gramine.
libos/src/fs/etc/fs.c line 39 at r1 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
It is ther:
size += strlen("search") + 1;// search + new_line
dns_host.dnsrch_count * (PAL_HOSTNAME_MAX + 1)-> size of hostname + 1 whitespace
Ah, this way. Got it.
libos/src/fs/etc/fs.c line 41 at r1 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
32 is covering all of it here, but I will do this more explicate maybe.
Yes, please, make it more explicit.
libos/src/fs/etc/fs.c line 83 at r1 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
This is correct formatting, and I think it is easier to generate it.
But If you really want I can change it.
How are you sure that this is correct formatting? Could you give me a link to the spec or e.g. Glibc resolver code?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 58 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @oshogbo)
Documentation/manifest-syntax.rst line 161 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please re-wrap to 80 chars per limit (we use 80 for documentation, 100 for code)
Oh, I was mislead because log level section is a little bit longer.
Sorry for that.
Documentation/manifest-syntax.rst line 161 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please add a sentence that all other keywords/formats are ignored, with warning messages.
Done.
libos/src/fs/etc/fs.c line 21 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I mean, it can if there is a very-very long option name that doesn't fit into 32 chars. This overestimation is not precise and may not cover all cases in the real world, and we end up with buffer overflows in production mode of Gramine.
Yea, but there isn't any very-very long option name, because it is fixed list. 32 chars are defined for inet6 (14 characters) and rotate (15 characters), so when you add new option you still have to add space for it and you can add 32 chars or 128 chars or whatever size you need, as this is a boolean filed. Besides the HOSTNAME, which we check during the pal we don't paste anything direct from user, every option length (at least for now is controlled by us this is why I check this with assert).
Please check current version, maybe it is more readable.
libos/src/fs/etc/fs.c line 41 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Yes, please, make it more explicit.
Done.
libos/src/fs/etc/fs.c line 83 at r1 (raw file):
Glibc resolver code
Sure.
https://elixir.bootlin.com/glibc/latest/source/resolv/res_init.c#L508
getline is called for each loop, and the setoptions are not clearing previous options.
pal/include/pal/pal.h line 37 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
maybe,
..., used in resolv.conf emulation
Done.
pal/include/pal/pal.h line 38 at r1 (raw file):
namespace
I took the names from the libresolv, but maybe we just call it PAL_MAN_NAMESPACE, and make it obvious without comment.
pal/include/pal/pal.h line 39 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I would add a comment that this is the max number of
searchlocal domain names
ditto.
pal/include/pal/pal.h line 107 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I would add a top comment here
/* Used in resolv.conf emulation */
Done.
pal/src/host/linux/pal_main.c line 130 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I would prefer the full name
/etc/resolv.confAlso, maybe
get->parse?
Done.
Code quote:
get_resolv_confpal/src/host/linux/pal_main.c line 423 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please align to the beginning of the first arg.
Done.
pal/src/host/linux/pal_main.c line 428 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Get host information->Get host /etc information
Done.
pal/src/host/linux-common/etc_host_info.c line 7 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I would add smth like this:
/* * This file contains the APIs to expose host information: * - parses host file `/etc/resolv.conf` into `struct pal_dns_host_conf`And in follow up PRs (if any), add more items to this list
Done.
pal/src/host/linux-common/etc_host_info.c line 10 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Do you even use this header?
Done.
pal/src/host/linux-common/etc_host_info.c line 16 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
You should remove this one and use
PAL_HOSTNAME_MAX
Done.
pal/src/host/linux-common/etc_host_info.c line 40 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
*conf-- wrong alignment
Done.
pal/src/host/linux-common/etc_host_info.c line 40 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
It would be better to have
static bool parse_ip_addr_ipv4(const char* ptr, uint32_t* addr), and move the logic of updating the DNS info and buffer-pointer into the caller
Done.
pal/src/host/linux-common/etc_host_info.c line 41 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
What is this word
octed? What's it supposed to mean?
Should be octet.
pal/src/host/linux-common/etc_host_info.c line 45 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Move
ito under the for loop
Sorry I don't understand.
pal/src/host/linux-common/etc_host_info.c line 50 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
If we return
false, we don't update*pptr. Doesn't it mean that we'll get stuck forever?Same for all other such places.
No, after failed and succeed parsing we still jump to the end of line no matters what.
This is useful; if you have multiple options in single line, here doesn't matter so much, but I wanted to be coherent.
pal/src/host/linux-common/etc_host_info.c line 66 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Where did you get these rules?
https://linux.die.net/man/3/inet_aton
a.b.c.d
Each of the four numeric parts specifies a byte of the address; the bytes are assigned in left-to-right order to produce the binary address.
a.b.c
Parts a and b specify the first two bytes of the binary address. Part c is interpreted as a 16-bit value that defines the rightmost two bytes of the binary address. This notation is suitable for specifying (outmoded) Class B network addresses.
a.b
Part a specifies the first byte of the binary address. Part b is interpreted as a 24-bit value that defines the rightmost three bytes of the binary address. This notation is suitable for specifying (outmoded) Class C network addresses.
a
The value a is interpreted as a 32-bit value that is stored directly into the binary address without any byte rearrangement.
Easy way to test it:
$ ping 8
PING 8 (0.0.0.8): 56 data bytes
$ ping 8.8
PING 8.8 (8.0.0.8): 56 data bytes
$ ping 8.8.8
PING 8.8.8 (8.8.0.8): 56 data bytes
$ ping 8.8.8.8
PING 8.8.8.8 (8.8.8.8): 56 data bytes
pal/src/host/linux-common/etc_host_info.c line 69 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This doesn't correspond to the actual logic? The logic seems to do
X.Y.0.Z
Sorry, comment is wrong.
Done.
pal/src/host/linux-common/etc_host_info.c line 71 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
What if we have
i == 0? Even if it'sAddress X has to be converted to: 0.0.0.X, I think it should be mentioned somewhere in comments
Done.
pal/src/host/linux-common/etc_host_info.c line 73 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Can we add an
assert(conf->nsaddr_list_count < PAL_MAXNS)
After changing the declaration of function this doesn't make sense.
pal/src/host/linux-common/etc_host_info.c line 84 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please make it a list:
* Supported notations: * - Full IPv6 address * - Abbreviations `::1`, `ff::1:2`
Done.
pal/src/host/linux-common/etc_host_info.c line 86 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
It would be better to have
static bool parse_ip_addr_ipv6(const char* ptr, uint16_t* addr), and move the logic of updating the DNS info and buffer-pointer into the caller
Done.
pal/src/host/linux-common/etc_host_info.c line 90 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please move to the first usages
Not sure, what you mean.
pal/src/host/linux-common/etc_host_info.c line 102 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why is this so? Why do we fail here?
Done.
pal/src/host/linux-common/etc_host_info.c line 106 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I'm lost, I don't understand how this parsing works. Could you please explain in the comments a bit?
Can
ibecome negative here?
I is the index of current 16 bits in IPv6.
If the IP address uses Abbreviations, we save the position where two dots are.
Thanks to that we know where we have to put 0 later on.
Yes, the i might become negative here.
So we have 8 slots.
If the IPv6 address will be for example `::ABCD:EFGA'
Then we will detect two dots at the begging (twocomas = 0)
pal/src/host/linux-common/etc_host_info.c line 130 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ican be greater than 7?
7 or 8 (we can end the for loop with 8 as the for loop is (i < 8)).
pal/src/host/linux-common/etc_host_info.c line 139 at r1 (raw file):
assert(conf->nsaddr_list_count < PAL_MAXNS)
Ditto.
pal/src/host/linux-common/etc_host_info.c line 152 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I'm pretty sure Gramine hasn't yet loaded
loader.log_levelmanifest option, so the current log level is anyway ERROR. Which means that this WARNING won't be printed at all, never.Please verify my assumption (e.g., in a Docker container with modified
/etc/resolv.conf), but I'm pretty sure.So, we should use
log_error()in this file, everywhere.UPDATE: Actually, I think Linux-SGX PAL should work fine, but Linux PAL probably not...
Oh you are right.
Thats a little bit confusing, especially as non of this things are actual erros.
pal/src/host/linux-common/etc_host_info.c line 152 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
then->than
Done.
pal/src/host/linux-common/etc_host_info.c line 157 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
os->is
Done.
pal/src/host/linux-common/etc_host_info.c line 159 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
contains(2x)
Done.
pal/src/host/linux-common/etc_host_info.c line 163 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Shouldn't you also break here? It's obvious at this point that this is IPv6
Done.
pal/src/host/linux-common/etc_host_info.c line 247 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
set_valuewould be a better name...
Done.
pal/src/host/linux-common/etc_host_info.c line 259 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
You use
\0in other places, please also use in this function
Ah Borys prefer 0x00 I will change it to it.
pal/src/host/linux-common/etc_host_info.c line 264 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
? Did you mean
startline == ptr(without asterisks)?
Ou, thats a very nice catch, thank you.
pal/src/host/linux-common/etc_host_info.c line 270 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Don't you have a buffer overread if
ptris somewhere on a very long word (e.g.ptrpoints tosuperlongword). In this case we overreadresolv_keys[i].keyword...I think you should check the
strlen(resolv_keys[i].keyword)first.
Why?
I mean the keyword is of fixed length and is for sure a null terminated, so strncmp(superlongword, "sth\0", size_of_superlongword) will compare \0 with 'e' and return that the string isn't equals right?
It just avoid calling strlen for each string over and over, but I guess it's micro optimization so if it will help in more readable code then I can change this.
pal/src/host/linux-common/etc_host_info.c line 271 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
We have a char limit of 100-per-line, but you seem to have smth like 90?
Stupid clang-formater.
pal/src/host/linux-common/etc_host_info.c line 296 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why don't we error out on parsing errors? We currently may have a weird case when
/etc/resolv.confwas garbled, and Gramine exposes/etc/resolv.confthat is empty... Is this our intention?
Good question.
The problem is that even if parse_resolv_conf will return some data they can be thrown away by PAL later on and we still can end with empty /etc/resolv.conf.
I don't know to be honest what should be the correct behavior.
pal/src/host/linux-common/etc_host_info.c line 180 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Maybe
parse_values_list_in_one_line()?
Done.
pal/src/host/linux-common/etc_host_info.c line 201 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
then->than
Done.
pal/src/host/linux-common/etc_host_info.c line 201 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
to long->too long
Done.
pal/src/host/linux-common/etc_host_info.c line 201 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Actually, can you print the domain name here?
Not really?
I mean the domain is not null terminated and arbitrary size, so I would have to create some buffer for it.
Dunno if its worth?
pal/src/host/linux-sgx/host_ecalls.h line 10 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Didn't @boryspoplawski want this to be in two arguments -- one for
topo_infoand another foretc_host_info?
Eh, I was hopping that we will get this merged together, but if you are unhappy with that then I will change that.
Done.
pal/src/host/linux-sgx/host_main.c line 632 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Wrong alignment of
*
Done.
pal/src/host/linux-sgx/host_main.c line 632 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please call it
out_emulate_etc
Done.
pal/src/host/linux-sgx/pal_ecall_types.h line 23 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I think it belongs to
pal_host.hBut also, I would just split this into two args, instead of adding everything in this one arg. This is also what @boryspoplawski wanted.
Ditto.
Done.
pal/src/host/linux-sgx/pal_linux.h line 25 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This won't be needed, see my other comment.
Done.
pal/src/host/linux-sgx/pal_main.c line 301 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
There is no need in
shallow_because you don't access any nested objects (no nested pointers) in this struct with regards to/etc/info.
Yes, but it doesn't point out also that these data has to be sanitized (like boolean values for example) ?
pal/src/host/linux-sgx/pal_main.c line 303 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why do you even need these helper variables? Just use normal variables. Yes, the names are longer, but much easier to read the code then.
I used long names, and this code looks very messy and confusing, especially with memcpy:
memcpy(&g_pal_public_state.dns_host->nsaddr_list[i].ipv6, &shallow_host_info->dns_host->nsaddr_list[i].ipv6, sizeof(g_pal_public_state.dns_host->nsaddr_list[i].ipv6));
Then I decided to use these helpers.
We use similar helpers in topo code.
But if you really want, I can change that.
pal/src/host/linux-sgx/pal_main.c line 312 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please add empty line after this one
Done.
pal/src/host/linux-sgx/pal_main.c line 316 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
binary
addresses
Done.
pal/src/host/linux-sgx/pal_main.c line 334 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Actually, I would print the name here.
Done.
pal/src/host/linux-sgx/pal_main.c line 680 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Yep, I definitely dislike that you rearranged a lot of code here, in Linux-SGX PAL's main func. Let's not move
topo_infoexisting code around, but rather add a new argument foretc_infoand new code.
Done.
There was a problem hiding this comment.
Reviewed 5 of 19 files at r1, 2 of 13 files at r3, all commit messages.
Reviewable status: 8 of 19 files reviewed, 65 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @oshogbo)
-- commits line 2 at r3:
I'd drop "Docs", it's implied by the change itself - i.e. when we add a feature to e.g. LibOS then we don't say that we modified LibOS and Docs, just LibOS ;) (and use "Docs" only for documentation-only commits)
Suggestion:
[LibOS,PAL]
Suggestion:
from the host
Documentation/manifest-syntax.rst line 156 at r3 (raw file):
(Default: false) This is achieved by taking the host's ``/etc`` files, parsing them, and
This reads weirdly, what is achieved by this? First sentence / paragraph should be about that "this" which implementation you describe here. I.e. start with saying that this enables emulation of <list here> files under etc.
Code quote:
This is achieved byDocumentation/manifest-syntax.rst line 156 at r3 (raw file):
(Default: false) This is achieved by taking the host's ``/etc`` files, parsing them, and
This is valid only assuming Linux PALs, on other PALs it will work differently (query a different OS API to gather this info).
Code quote:
by taking the host's ``/etc`` filesDocumentation/manifest-syntax.rst line 178 at r3 (raw file):
This option takes precedence over ``fs.mounts``. This means that etc files provided via ``fs.mounts`` will be overridden with the ones sanitized by LibOS.
Suggestion:
the ones added via this optionlibos/src/fs/etc/fs.c line 21 at r1 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
Yea, but there isn't any very-very long option name, because it is fixed list. 32 chars are defined for inet6 (14 characters) and rotate (15 characters), so when you add new option you still have to add space for it and you can add 32 chars or 128 chars or whatever size you need, as this is a boolean filed. Besides the HOSTNAME, which we check during the pal we don't paste anything direct from user, every option length (at least for now is controlled by us this is why I check this with assert).
Please check current version, maybe it is more readable.
I think I understand @oshogbo's point, but it still seems a bit dangerous for me if we miscalculate something. We have that assert, but it can only detect the inputs which we actually test.
IMO making this assert an if cost nothing and makes this code less error prone.
libos/src/fs/etc/fs.c line 83 at r1 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
Glibc resolver code
Sure.https://elixir.bootlin.com/glibc/latest/source/resolv/res_init.c#L508
getline is called for each loop, and the setoptions are not clearing previous options.
The docs are unclear on this, but on the other hand it makes the code much simpler. No strong preferences from my side.
libos/src/fs/etc/fs.c line 46 at r3 (raw file):
size += (g_pal_public_state->dns_host.inet6 ? strlen(OPTION_INET6) : 0) + (g_pal_public_state->dns_host.rotate ? strlen(OPTION_ROTATE) : 0); /* snprintf adds the terminating character */
snprintf has nothing to do here, this is just a standard C string NULL termination? The size until now didn't accommodate for it, so we needed to add 1.
Code quote:
snprintf adds the terminating characterlibos/src/fs/etc/fs.c line 116 at r3 (raw file):
int init_etcfs(void) { pseudo_add_str(NULL, "emulate-etc-resolv-conf", &provide_etc_resolv_conf);
(this is an object name, not a function name)
Suggestion:
"emulated-etc-resolv-conf"
There was a problem hiding this comment.
Reviewed 6 of 13 files at r3, all commit messages.
Reviewable status: 12 of 19 files reviewed, 30 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @mkow, and @oshogbo)
libos/src/fs/etc/fs.c line 21 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I think I understand @oshogbo's point, but it still seems a bit dangerous for me if we miscalculate something. We have that assert, but it can only detect the inputs which we actually test.
IMO making this assert anifcost nothing and makes this code less error prone.
I agree with @mkow, I would feel safer with an if check.
libos/src/fs/etc/fs.c line 83 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
The docs are unclear on this, but on the other hand it makes the code much simpler. No strong preferences from my side.
Thanks @oshogbo for the link. I'm satisfied now.
pal/include/pal/pal.h line 38 at r1 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
namespace
I took the names from the
libresolv, but maybe we just call it PAL_MAN_NAMESPACE, and make it obvious without comment.
+1 for a better name
pal/include/pal/pal.h line 39 at r1 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
ditto.
+1 for a better name
pal/src/host/linux-common/etc_host_info.c line 40 at r1 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
Done.
I see that you you still update *pptr (buffer-pointer) in this function instead of in the caller. Any particular reason? Well, I'm ok with it.
pal/src/host/linux-common/etc_host_info.c line 45 at r1 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
Sorry I don't understand.
Ignore me, I didn't realize we use i after the for loop
pal/src/host/linux-common/etc_host_info.c line 50 at r1 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
No, after failed and succeed parsing we still jump to the end of line no matters what.
This is useful; if you have multiple options in single line, here doesn't matter so much, but I wanted to be coherent.
Ah, right. I suggested to add the comment that "we still jump to the end of line no matter what" in that place in code, please add it. Resolving here.
pal/src/host/linux-common/etc_host_info.c line 66 at r1 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
https://linux.die.net/man/3/inet_aton
a.b.c.d Each of the four numeric parts specifies a byte of the address; the bytes are assigned in left-to-right order to produce the binary address. a.b.c Parts a and b specify the first two bytes of the binary address. Part c is interpreted as a 16-bit value that defines the rightmost two bytes of the binary address. This notation is suitable for specifying (outmoded) Class B network addresses. a.b Part a specifies the first byte of the binary address. Part b is interpreted as a 24-bit value that defines the rightmost three bytes of the binary address. This notation is suitable for specifying (outmoded) Class C network addresses. a The value a is interpreted as a 32-bit value that is stored directly into the binary address without any byte rearrangement.Easy way to test it:
$ ping 8 PING 8 (0.0.0.8): 56 data bytes $ ping 8.8 PING 8.8 (8.0.0.8): 56 data bytes $ ping 8.8.8 PING 8.8.8 (8.8.0.8): 56 data bytes $ ping 8.8.8.8 PING 8.8.8.8 (8.8.8.8): 56 data bytes
Thanks! I didn't know this.
pal/src/host/linux-common/etc_host_info.c line 90 at r1 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
Not sure, what you mean.
You've done it for twocomas already, thanks. And regarding i, it was my mistake (I thought that i was used only inside the for loop)
pal/src/host/linux-common/etc_host_info.c line 106 at r1 (raw file):
Yes, the i might become negative here.
How is this safe? So we can legitimately have i = -1, and then below we have addr[i] = part;. What prevents your logic from having a buffer overflow?
pal/src/host/linux-common/etc_host_info.c line 130 at r1 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
7 or 8 (we can end the for loop with 8 as the for loop is (i < 8)).
But i == 8 should be impossible, no? Because for well-formed IPv6 addresses, we'll trigger if (is_end_of_word(*next)) break;, so the for loop will not update i to 8. And if the address was ill-formed, then i == 8 indeed but we just return false; immediately after the loop.
pal/src/host/linux-common/etc_host_info.c line 152 at r1 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
Oh you are right.
Thats a little bit confusing, especially as non of this things are actual erros.
Yes, it's confusing. But I don't think we can do much about it. Also, I think it's fine that they are always printed (and marked as errors), because this means that something is really wrong with /etc/resolv.conf.
pal/src/host/linux-common/etc_host_info.c line 270 at r1 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
Why?
I mean the keyword is of fixed length and is for sure a null terminated, so strncmp(superlongword, "sth\0", size_of_superlongword) will compare \0 with 'e' and return that the string isn't equals right?It just avoid calling strlen for each string over and over, but I guess it's micro optimization so if it will help in more readable code then I can change this.
You are right, it's me being stupid. No need to change this then.
pal/src/host/linux-common/etc_host_info.c line 296 at r1 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
Good question.
The problem is that even ifparse_resolv_confwill return some data they can be thrown away by PAL later on and we still can end with empty/etc/resolv.conf.
I don't know to be honest what should be the correct behavior.
True. Let's see what @boryspoplawski and @mkow will say on this.
pal/src/host/linux-common/etc_host_info.c line 201 at r2 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
Not really?
I mean the domain is not null terminated and arbitrary size, so I would have to create some buffer for it.
Dunno if its worth?
Ok, true.
pal/src/host/linux-common/etc_host_info.c line 71 at r3 (raw file):
} /* i == 4 Address A.X.Y.Z, nothing to do. */ assert(i <= 4);
i == 4 should be impossible, no? Because for well-formed A.X.Y.Z addresses, we'll trigger if (is_end_of_word(*next)) break;, so the for loop will not update i to 4. And if the address was ill-formed, then i == 4 indeed but we just return false; immediately after the loop.
pal/src/host/linux-common/etc_host_info.c line 99 at r3 (raw file):
if (i == 0) { /* The IPv6 address starts with the ':', this means that the next character * has to be also ':' (tu support ::something), otherwise it is invalid
tu -> to
pal/src/host/linux-common/etc_host_info.c line 100 at r3 (raw file):
/* The IPv6 address starts with the ':', this means that the next character * has to be also ':' (tu support ::something), otherwise it is invalid * IPv6 adders (:something). When i > 0 we now already that the previous
adders -> address
pal/src/host/linux-common/etc_host_info.c line 100 at r3 (raw file):
/* The IPv6 address starts with the ':', this means that the next character * has to be also ':' (tu support ::something), otherwise it is invalid * IPv6 adders (:something). When i > 0 we now already that the previous
now -> know
pal/src/host/linux-common/etc_host_info.c line 190 at r3 (raw file):
static void parse_values_list_in_one_line(struct pal_dns_host_conf* conf, const char** pptr, void (*setter)(struct pal_dns_host_conf*, const char*, size_t)) {
You need to also re-align this argument. Now I think that my proposed super-long func name was not a good idea.
pal/src/host/linux-common/etc_host_info.c line 290 at r3 (raw file):
} } /* Make sure we are at the end of line. */
Maybe add here ..., even if parsing of this line failed
There was a problem hiding this comment.
Reviewed 7 of 13 files at r3.
Reviewable status: all files reviewed, 26 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @mkow, and @oshogbo)
pal/src/host/linux-sgx/host_main.c line 632 at r1 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
Done.
You understood me wrong :)
I meant to change the arg name emulated_etc -> out_emulated_etc, not the function name.
But actually, out_emulate_etc is also wrong here -- we add out_ prefix for those objects that are created inside the function (like strings). Sorry for the confusion, please revert all these renames back.
pal/src/host/linux-sgx/pal_main.c line 301 at r1 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
Yes, but it doesn't point out also that these data has to be sanitized (like boolean values for example) ?
You modified this function anyway, so now my comment is irrelevant.
pal/src/host/linux-sgx/pal_main.c line 303 at r1 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
I used long names, and this code looks very messy and confusing, especially with
memcpy:memcpy(&g_pal_public_state.dns_host->nsaddr_list[i].ipv6, &shallow_host_info->dns_host->nsaddr_list[i].ipv6, sizeof(g_pal_public_state.dns_host->nsaddr_list[i].ipv6));Then I decided to use these helpers.
We use similar helpers in topo code.
But if you really want, I can change that.
Ok, you're right, looks ugly.
pal/src/host/linux-sgx/pal_main.c line 334 at r1 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
Done.
The search entire -> The search domain name
(Or I misunderstood what you wanted to say in this message?)
pal/src/host/linux-sgx/pal_main.c line 320 at r3 (raw file):
sizeof(*uptr_dns_conf))) { log_error("Unable to read host info"); ocall_exit(1, /*is_exitgroup=*/true);
You should return -EINVAL; instead.
pal/src/host/linux-sgx/pal_main.c line 324 at r3 (raw file):
if (shallow_dns.nsaddr_list_count > PAL_MAXNS) { log_error("To many nameservers provided");
To -> Too
pal/src/host/linux-sgx/pal_main.c line 343 at r3 (raw file):
if (shallow_dns.dnsrch_count > PAL_MAXDNSRCH) { log_error("To many search entries provided");
To -> Too
pal/src/host/linux-sgx/pal_main.c line 690 at r3 (raw file):
ret = import_and_sanitize_topo_info(uptr_topo_info); if (ret < 0) { log_error("Failed to copy and sanitize topology information");
Please add the error code itself: ...: %d", ret);
pal/src/host/linux-sgx/pal_main.c line 754 at r3 (raw file):
if (parent_stream_fd < 0) { if ((ret = import_and_init_emulation_etc_files(uptr_dns_conf)) < 0) { log_error("Failed to initialize host info");
Please add the error code itself: ...: %d", ret);
There was a problem hiding this comment.
Reviewed 1 of 19 files at r1, 11 of 13 files at r3.
Reviewable status: all files reviewed, 50 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @oshogbo)
pal/include/pal/pal.h line 38 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
+1 for a better name
PAL_MAN_NAMESPACE -> PAL_MAX_NAMESPACES
pal/include/pal/pal.h line 110 at r3 (raw file):
struct pal_dns_host_conf { struct pal_dns_host_conf_addr nsaddr_list[PAL_MAXNS]; size_t nsaddr_list_count;
I'd unindent this, looks too weird to me :)
pal/src/host/linux/pal_main.c line 130 at r3 (raw file):
if (parse_resolv_conf(&g_pal_public_state.dns_host) < 0) { INIT_FAIL("Unable to get /etc/resolv.conf");
Suggestion:
Unable to parse /etc/resolv.confpal/src/host/linux-common/etc_host_info.c line 66 at r1 (raw file):
Part b is interpreted as a 24-bit value that defines the rightmost three bytes of the binary address.
The current code doesn't seem to match this? Ditto below.
pal/src/host/linux-common/etc_host_info.c line 296 at r1 (raw file):
they can be thrown away by PAL later
What do you mean? Why would it throw them out?
pal/src/host/linux-common/etc_host_info.c line 7 at r3 (raw file):
/* * This file contains the APIs to expose host information:
Suggestion:
APIs to retrieve network information from the hostpal/src/host/linux-common/etc_host_info.c line 37 at r3 (raw file):
} static bool parse_ip_addr_ipv4(const char** pptr, uint32_t* paddr) {
Suggestion:
out_addrpal/src/host/linux-common/etc_host_info.c line 62 at r3 (raw file):
if (!is_end_of_word(*next)) return false; /* i == 0 Address X has to be converted to: 0.0.0.X, nothing to do. */
Suggestion:
i == 0: Address X has to be pal/src/host/linux-common/etc_host_info.c line 70 at r3 (raw file):
addr = ((addr & 0xFFFF00) << 8) | (addr & 0xFF); } /* i == 4 Address A.X.Y.Z, nothing to do. */
Suggestion:
i == 4: Address A.X.Y.Z, nothing to do.pal/src/host/linux-common/etc_host_info.c line 88 at r3 (raw file):
const char* ptr = *pptr; char* next; int i, twocomas = -1;
Suggestion:
two_colonspal/src/host/linux-common/etc_host_info.c line 99 at r3 (raw file):
if (i == 0) { /* The IPv6 address starts with the ':', this means that the next character * has to be also ':' (tu support ::something), otherwise it is invalid
otherwise it is invalid IPv6 adders -> otherwise it is invalid
pal/src/host/linux-common/etc_host_info.c line 191 at r3 (raw file):
static void parse_values_list_in_one_line(struct pal_dns_host_conf* conf, const char** pptr, void (*setter)(struct pal_dns_host_conf*, const char*, size_t)) { const char* ptr = *pptr;
please unalign, looks ugly
pal/src/host/linux-common/etc_host_info.c line 239 at r3 (raw file):
if (length == 0) return; if (length >= sizeof(option))
Could we log an error here?
pal/src/host/linux-common/etc_host_info.c line 257 at r3 (raw file):
static struct { const char* keyword; void (*set_value)(struct pal_dns_host_conf* conf, const char** pptr);
ditto, please unalign
pal/src/host/linux-common/etc_host_info.c line 266 at r3 (raw file):
static void parse_resolv_buf_conf(struct pal_dns_host_conf* conf, const char* buf) { const char* ptr = buf;
ditto (alignment)
pal/src/host/linux-common/etc_host_info.c line 281 at r3 (raw file):
for (size_t i = 0; resolv_keys[i].keyword != NULL; i++) { if (strncmp(startline, resolv_keys[i].keyword, ptr - startline - 1) == 0) { /* Because the buffer in strncmp is not ended with 0x00, lets
Suggestion:
let'spal/src/host/linux-common/etc_host_info.c line 282 at r3 (raw file):
if (strncmp(startline, resolv_keys[i].keyword, ptr - startline - 1) == 0) { /* Because the buffer in strncmp is not ended with 0x00, lets * verify that the length of keywords are the same. */
(or are -> is)
Suggestion:
lengths of keywords are the samepal/src/host/linux-sgx/enclave_ecalls.c line 102 at r3 (raw file):
struct pal_dns_host_conf* dns_conf = READ_ONCE(ms->ms_dns_host_conf); if (!dns_conf || !sgx_is_completely_outside_enclave(dns_conf, sizeof(*dns_conf)))
What's the point of this check?
Code quote:
!dns_confpal/src/host/linux-sgx/host_ecalls.h line 6 at r3 (raw file):
#include "sgx_arch.h" #include "pal_topology.h"
Why have you removed this include? We still use struct pal_topo_info from it.
pal/src/host/linux-sgx/pal_main.c line 334 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
The search entire->The search domain name(Or I misunderstood what you wanted to say in this message?)
I also don't understand the current message.
pal/src/host/linux-sgx/pal_main.c line 265 at r3 (raw file):
* - the hostname doesn't start or end with '.' and '-', * - the hostname contains only alphanumeric characters, '-', and '.', * These rules were deducted from:
"deducted"?
pal/src/host/linux-sgx/pal_main.c line 284 at r3 (raw file):
chrcount++; ptr++; continue;
Wrong indentation
pal/src/host/linux-sgx/pal_main.c line 286 at r3 (raw file):
continue; } else if (*ptr == '.') { if (chrcount == 0 || chrcount > 63) {
Wrong indentation
pal/src/host/linux-sgx/pal_main.c line 316 at r3 (raw file):
return 0; struct pal_dns_host_conf shallow_dns = {0};
No need for this, you literally overwrite it on the next line ;)
Code quote:
= {0}pal/src/host/linux-sgx/pal_main.c line 331 at r3 (raw file):
for (i = 0; i < pub_dns->nsaddr_list_count; i++) { coerce_untrusted_bool(&shallow_dns.nsaddr_list[i].is_ipv6); /* All binary IP addresses are already valid ones. */
Suggestion:
validpal/src/host/linux-sgx/pal_main.c line 354 at r3 (raw file):
memcpy(pub_dns->dnsrch[j], shallow_dns.dnsrch[i], sizeof(pub_dns->dnsrch[j]) - 1);
no need for wrapping
pal/src/host/linux-sgx/pal_main.c line 744 at r3 (raw file):
ret = toml_bool_in(g_pal_public_state.manifest_root, "libos.emulate_etc_files", false, &g_pal_public_state.emulate_etc_files);
wrong alignment
There was a problem hiding this comment.
Reviewable status: 12 of 19 files reviewed, 50 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "squash! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @mkow, and @oshogbo)
Previously, mkow (Michał Kowalczyk) wrote…
I'd drop "Docs", it's implied by the change itself - i.e. when we add a feature to e.g. LibOS then we don't say that we modified LibOS and Docs, just LibOS ;) (and use "Docs" only for documentation-only commits)
Done.
-- commits line 4 at r3:
Done.
Documentation/manifest-syntax.rst line 156 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
This reads weirdly, what is achieved by this? First sentence / paragraph should be about that "this" which implementation you describe here. I.e. start with saying that this enables emulation of <list here> files under etc.
Done.
Documentation/manifest-syntax.rst line 156 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
This is valid only assuming Linux PALs, on other PALs it will work differently (query a different OS API to gather this info).
Done.
Documentation/manifest-syntax.rst line 178 at r3 (raw file):
This option takes precedence over ``fs.mounts``. This means that etc files provided via ``fs.mounts`` will be overridden with the ones sanitized by LibOS.
Done.
libos/src/fs/etc/fs.c line 21 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I agree with @mkow, I would feel safer with an
ifcheck.
Done.
libos/src/fs/etc/fs.c line 46 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
snprintfhas nothing to do here, this is just a standard C string NULL termination? The size until now didn't accommodate for it, so we needed to add 1.
Done.
libos/src/fs/etc/fs.c line 116 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
(this is an object name, not a function name)
Done.
pal/include/pal/pal.h line 38 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
PAL_MAN_NAMESPACE->PAL_MAX_NAMESPACES
Done.
pal/include/pal/pal.h line 39 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
+1 for a better name
Done.
pal/include/pal/pal.h line 110 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I'd unindent this, looks too weird to me :)
Done.
pal/src/host/linux/pal_main.c line 130 at r3 (raw file):
if (parse_resolv_conf(&g_pal_public_state.dns_host) < 0) { INIT_FAIL("Unable to get /etc/resolv.conf");
Done.
pal/src/host/linux-common/etc_host_info.c line 66 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Part b is interpreted as a 24-bit value that defines the rightmost three bytes of the binary address.
The current code doesn't seem to match this? Ditto below.
Ugh you are right.
Nice catch.
Which code is better the committed one or this one:
int j;
uint32_t bits = 0xFFFFFFFF;
for (j = 0; j < i; j++) {
if (addr[j] > 0xFF)
return false;
result |= addr[j] << (24 - j * 8);
bits >>= 8;
}
if (addr[i] > bits)
return false;
result |= addr[i];
This is a raw idea, I haven't tested the code above (I have tested the committed code).
pal/src/host/linux-common/etc_host_info.c line 106 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Yes, the i might become negative here.
How is this safe? So we can legitimately have
i = -1, and then below we haveaddr[i] = part;. What prevents your logic from having a buffer overflow?
We continue, so we do i++ and we back to 0.
pal/src/host/linux-common/etc_host_info.c line 130 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
But
i == 8should be impossible, no? Because for well-formed IPv6 addresses, we'll triggerif (is_end_of_word(*next)) break;, so the for loop will not updateito 8. And if the address was ill-formed, theni == 8indeed but we justreturn false;immediately after the loop.
Yes, that true, but i would prefer to lave this just in case.
pal/src/host/linux-common/etc_host_info.c line 7 at r3 (raw file):
/* * This file contains the APIs to expose host information:
Currently it is network, but I think there will be more then network.
pal/src/host/linux-common/etc_host_info.c line 37 at r3 (raw file):
} static bool parse_ip_addr_ipv4(const char** pptr, uint32_t* paddr) {
Done.
pal/src/host/linux-common/etc_host_info.c line 62 at r3 (raw file):
if (!is_end_of_word(*next)) return false; /* i == 0 Address X has to be converted to: 0.0.0.X, nothing to do. */
Done.
pal/src/host/linux-common/etc_host_info.c line 70 at r3 (raw file):
addr = ((addr & 0xFFFF00) << 8) | (addr & 0xFF); } /* i == 4 Address A.X.Y.Z, nothing to do. */
Done.
pal/src/host/linux-common/etc_host_info.c line 71 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
i == 4should be impossible, no? Because for well-formedA.X.Y.Zaddresses, we'll triggerif (is_end_of_word(*next)) break;, so the for loop will not updateito 4. And if the address was ill-formed, theni == 4indeed but we justreturn false;immediately after the loop.
Yes, it should be.
pal/src/host/linux-common/etc_host_info.c line 88 at r3 (raw file):
const char* ptr = *pptr; char* next; int i, twocomas = -1;
Done.
pal/src/host/linux-common/etc_host_info.c line 99 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
tu->to
Done.
pal/src/host/linux-common/etc_host_info.c line 99 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
otherwise it is invalid IPv6 adders->otherwise it is invalid
Done.
pal/src/host/linux-common/etc_host_info.c line 100 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
adders->address
Done.
pal/src/host/linux-common/etc_host_info.c line 100 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
now->know
Done.
pal/src/host/linux-common/etc_host_info.c line 190 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
You need to also re-align this argument. Now I think that my proposed super-long func name was not a good idea.
With the new name this is longer then 100 lines, not sure how I should split it.
pal/src/host/linux-common/etc_host_info.c line 191 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
please unalign, looks ugly
Done.
pal/src/host/linux-common/etc_host_info.c line 239 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Could we log an error here?
We ignore unknown options, in this case we will print an error to unknown option.
pal/src/host/linux-common/etc_host_info.c line 257 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
ditto, please unalign
Done.
pal/src/host/linux-common/etc_host_info.c line 266 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
ditto (alignment)
Done.
pal/src/host/linux-common/etc_host_info.c line 281 at r3 (raw file):
for (size_t i = 0; resolv_keys[i].keyword != NULL; i++) { if (strncmp(startline, resolv_keys[i].keyword, ptr - startline - 1) == 0) { /* Because the buffer in strncmp is not ended with 0x00, lets
Done.
pal/src/host/linux-common/etc_host_info.c line 282 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
(or
are->is)
Done.
pal/src/host/linux-common/etc_host_info.c line 290 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Maybe add here
..., even if parsing of this line failed
Done.
pal/src/host/linux-sgx/enclave_ecalls.c line 102 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
What's the point of this check?
That the null wasn't provided (to be honest I copy it from topo).
pal/src/host/linux-sgx/host_ecalls.h line 6 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Why have you removed this include? We still use
struct pal_topo_infofrom it.
pal_topology.h is included by pal.h
pal/src/host/linux-sgx/host_main.c line 632 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
You understood me wrong :)
I meant to change the arg name
emulated_etc->out_emulated_etc, not the function name.But actually,
out_emulate_etcis also wrong here -- we addout_prefix for those objects that are created inside the function (like strings). Sorry for the confusion, please revert all these renames back.
Done.
pal/src/host/linux-sgx/pal_main.c line 334 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I also don't understand the current message.
Done.
pal/src/host/linux-sgx/pal_main.c line 265 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
"deducted"?
Done.
pal/src/host/linux-sgx/pal_main.c line 284 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Wrong indentation
Done.
pal/src/host/linux-sgx/pal_main.c line 286 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Wrong indentation
Done.
pal/src/host/linux-sgx/pal_main.c line 320 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
You should
return -EINVAL;instead.
Sorry?
pal/src/host/linux-sgx/pal_main.c line 324 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
To->Too
Done.
pal/src/host/linux-sgx/pal_main.c line 331 at r3 (raw file):
for (i = 0; i < pub_dns->nsaddr_list_count; i++) { coerce_untrusted_bool(&shallow_dns.nsaddr_list[i].is_ipv6); /* All binary IP addresses are already valid ones. */
Done.
pal/src/host/linux-sgx/pal_main.c line 343 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
To->Too
Done.
pal/src/host/linux-sgx/pal_main.c line 354 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
no need for wrapping
Done.
pal/src/host/linux-sgx/pal_main.c line 690 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please add the error code itself:
...: %d", ret);
Done.
pal/src/host/linux-sgx/pal_main.c line 744 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
wrong alignment
Done.
pal/src/host/linux-sgx/pal_main.c line 754 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please add the error code itself:
...: %d", ret);
Done.
There was a problem hiding this comment.
Reviewable status: 12 of 19 files reviewed, 50 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "squash! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @mkow, and @oshogbo)
pal/src/host/linux-common/etc_host_info.c line 296 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
they can be thrown away by PAL later
What do you mean? Why would it throw them out?
So if I understand correctly, @dimakuv his idea is to thrown an error if the /etc/resolv.conf on the host is empty or that we didn't parse any variable data.
First, I don't think that's a problem - some machines might not have a resolv.conf, and etc emulation might get other useful files (when we will support more of them).
Second, I don't think that's a proper place to do it. For example, we can parse only search keywords, and here we think that we have meaningful data. Later the Pal/SGX-Linux will sanitize it, and decides that domain in search keyword isn't really a domain - this is what I mean by "throw them out".
And we again might end up with empty /etc/resolv.conf. So probably best place would be libos/src/fs/etc/fs.c .
But what if this is actually a valid configuration that /etc/resolv.conf is empty?
There was a problem hiding this comment.
Reviewed 6 of 19 files at r1, 10 of 13 files at r3, 1 of 7 files at r4.
Reviewable status: 13 of 19 files reviewed, 69 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "squash! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @mkow, and @oshogbo)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
OK. I'm fine with this.
I think we'll need to create follow-up PRs with the following additional keywords immediately:
options edns0-- propagate into Gramineoptions trust-ad-- ignore with some clear log messagendots:n-- propagate into Gramineuse-vc-- propagate into Gramine
@dimakuv why the last two files? They were not mentioned before.
Also FWIW this is the list I've found on all servers I had access to (the sum of all seen options):
search,domain,nameserver,options ends0 trust-ad
Previously, mkow (Michał Kowalczyk) wrote…
I'd drop "Docs", it's implied by the change itself - i.e. when we add a feature to e.g. LibOS then we don't say that we modified LibOS and Docs, just LibOS ;) (and use "Docs" only for documentation-only commits)
+1, for some reason many people started doing this lately, I wonder why
Documentation/manifest-syntax.rst line 156 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
This is valid only assuming Linux PALs, on other PALs it will work differently (query a different OS API to gather this info).
+1, I have the same stipulation
IMO this should document the manifest syntax + what users get through it, i.e. what is exposed by LibOS to the user app.
Documentation/manifest-syntax.rst line 160 at r3 (raw file):
modes (such as SGX), Gramine additionally sanitizes the contents of these files. Note that Gramine's parsers for these files support only a subset of
It's not the parsers, it's whole Gramine. Also some potential PALs might not have any parsers at all.
libos/src/fs/etc/fs.c line 21 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I agree with @mkow, I would feel safer with an
ifcheck.
+1
libos/src/fs/etc/fs.c line 100 at r1 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
Done.
< since data must be null terminated
libos/src/fs/etc/fs.c line 16 at r3 (raw file):
#define OPTION_INET6 "options inet6\n" #define OPTION_ROTATE "options rotate\n"
What's with these weird indents?
libos/src/fs/etc/fs.c line 16 at r3 (raw file):
#define OPTION_INET6 "options inet6\n" #define OPTION_ROTATE "options rotate\n"
Do we want to keep these newlines here? Looks weird to me... Also other entries add \n manually
libos/src/fs/etc/fs.c line 39 at r3 (raw file):
/* Estimate the size of buffer: */ /* nameservers - lets assume all entries will be IPv6 plus a new line */ size += g_pal_public_state->dns_host.nsaddr_list_count * (strlen("nameserver ") + 40 + 1);
Please change 40 to some define, like MAX_IPV6_ADDR_LEN or something.
libos/src/fs/etc/fs.c line 39 at r3 (raw file):
/* Estimate the size of buffer: */ /* nameservers - lets assume all entries will be IPv6 plus a new line */ size += g_pal_public_state->dns_host.nsaddr_list_count * (strlen("nameserver ") + 40 + 1);
Could you remove the space from the string and just +1?
Maybe that would be cleaner only for me, so not blocking
libos/src/fs/etc/fs.c line 41 at r3 (raw file):
size += g_pal_public_state->dns_host.nsaddr_list_count * (strlen("nameserver ") + 40 + 1); /* search - lets assume maximum length of entries, plus a new line and white spaces */ size += strlen("search") + 1;
Let's move this +1 to separate line at the end of this section, so it's clear what it accounts for (a new line, which is after all dnsrch entries). Also IMO it would be good to match the order of accounting to the order we print these chars.
libos/src/fs/etc/fs.c line 55 at r3 (raw file):
/* Generate data: */ char* ptr = data; int ret = 0;
Suggestion:
int ret;libos/src/fs/etc/fs.c line 116 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
(this is an object name, not a function name)
I think this is just internal name / key to use for provide_etc_resolv_conf function, i.g. it's not an object.
pal/include/pal/pal.h line 104 at r3 (raw file):
bool is_ipv6; uint32_t ipv4; uint16_t ipv6[8];
Suggestion:
bool is_ipv6;
union {
uint32_t ipv4;
uint16_t ipv6[8];
};pal/include/pal/pal.h line 110 at r3 (raw file):
struct pal_dns_host_conf { struct pal_dns_host_conf_addr nsaddr_list[PAL_MAXNS]; size_t nsaddr_list_count;
I prefer length over count when talking about lists/arrays (because count of what, maybe _items_counts...)
As other reviewers didn't mention it I'm not blocking, maybe just personal preference.
pal/include/pal/pal.h line 112 at r3 (raw file):
size_t nsaddr_list_count; char dnsrch[PAL_MAXDNSRCH][PAL_HOSTNAME_MAX];
Could we have some meaningful name? I broke my tongue on dnsrch. Like dns_search isn't even that long...
pal/include/pal/pal.h line 113 at r3 (raw file):
char dnsrch[PAL_MAXDNSRCH][PAL_HOSTNAME_MAX]; size_t dnsrch_count;
ditto
pal/src/host/linux/pal_main.c line 422 at r3 (raw file):
} ret = toml_bool_in(g_pal_public_state.manifest_root, "libos.emulate_etc_files", false,
Suggestion:
/*defaultval=*/falsepal/src/host/linux-common/etc_host_info.c line 66 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Part b is interpreted as a 24-bit value that defines the rightmost three bytes of the binary address.
The current code doesn't seem to match this? Ditto below.
Yup, this code doesn't handle cases like 8.1024 == 8.0.4.0.
pal/src/host/linux-common/etc_host_info.c line 24 at r3 (raw file):
} static void skip_whitespace(const char** pptr) {
Suggestion:
skip_whitespaces(pal/src/host/linux-common/etc_host_info.c line 27 at r3 (raw file):
const char* ptr = *pptr; while (*ptr == ' ' || *ptr == '\t')
I didn't review the usages yet, but looking at other code around, shouldnt we also check for nullbyte?
pal/src/host/linux-common/etc_host_info.c line 33 at r3 (raw file):
} static bool is_end_of_word(const char ch) {
Suggestion:
char chpal/src/host/linux-common/etc_host_info.c line 42 at r3 (raw file):
char* next; uint32_t addr = 0; int i;
Suggestion:
size_t i;pal/src/host/linux-common/etc_host_info.c line 70 at r3 (raw file):
addr = ((addr & 0xFFFF00) << 8) | (addr & 0xFF); } /* i == 4 Address A.X.Y.Z, nothing to do. */
i == 3 actually.
pal/src/host/linux-common/etc_host_info.c line 88 at r3 (raw file):
const char* ptr = *pptr; char* next; int i, twocomas = -1;
Suggestion:
size_t i;
int two_colons = -1;pal/src/host/linux-common/etc_host_info.c line 91 at r3 (raw file):
uint16_t addr[8]; memset(&addr, 0, sizeof(addr));
Suggestion:
uint16_t addr[8] = { 0 };pal/src/host/linux-sgx/enclave_ecalls.c line 102 at r3 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
That the null wasn't provided (to be honest I copy it from topo).
This will need to be changed when rebasing anyway - now we handle all of this in pal_linux_main (instead of some random subset here).
This will be a trivial change tho.
pal/src/host/linux-sgx/host_ecalls.h line 6 at r3 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
pal_topology.his included bypal.h
But here we need it because we directly use stuff from it (struct pal_topo_info I assume).
pal/src/host/linux-sgx/host_main.c line 892 at r4 (raw file):
g_host_log_level = log_level; ret = toml_bool_in(manifest_root, "libos.emulate_etc_files", false, emulate_etc);
Suggestion:
/*defaultval=*/false,pal/src/host/linux-sgx/host_main.c line 895 at r4 (raw file):
if (ret < 0) { log_error("Cannot parse 'libos.emulate_etc_files'"); return ret;
Suggestion:
goto out;
There was a problem hiding this comment.
Reviewed 6 of 7 files at r4, all commit messages.
Reviewable status: all files reviewed, 92 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "squash! " found in commit messages' one-liners (waiting on @dimakuv, @mkow, and @oshogbo)
-- commits line 21 at r4:
This is wrong.
-- commits line 31 at r4:
2x sign-off
pal/include/pal/pal.h line 112 at r3 (raw file):
size_t nsaddr_list_count; char dnsrch[PAL_MAXDNSRCH][PAL_HOSTNAME_MAX];
If you don't indent, then do it everywhere :)
pal/src/host/linux-common/etc_host_info.c line 201 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Ok, true.
%.1337s should work
pal/src/host/linux-common/etc_host_info.c line 7 at r3 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
Currently it is network, but I think there will be more then network.
Why do you think so?
pal/src/host/linux-common/etc_host_info.c line 88 at r3 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
Done.
Not done
pal/src/host/linux-common/etc_host_info.c line 239 at r3 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
We ignore unknown options, in this case we will print an error to unknown option.
And that's bad because? Cannot we log unknown options?
pal/src/host/linux-common/etc_host_info.c line 38 at r4 (raw file):
static bool parse_ip_addr_ipv4(const char** pptr, uint32_t* out_addr) { long long octet;
Please move to usage
pal/src/host/linux-common/etc_host_info.c line 62 at r4 (raw file):
if (!is_end_of_word(*next)) return false;
Please add assert(i < 4);
pal/src/host/linux-common/etc_host_info.c line 65 at r4 (raw file):
uint32_t result = 0; if (i == 0) { /* Address A has to be converted to: A.A.A.A
What, this comment makes no sense. 8 is not converted to 8.8.8.8.
Ditto for all other comments.
pal/src/host/linux-common/etc_host_info.c line 101 at r4 (raw file):
* - Abbreviations `::1`, `ff::1:2` */ static bool parse_ip_addr_ipv6(const char** pptr, uint16_t* out_addr) {
I don't think this function is correct. Things like:
1::\r\r\r+11::0x121::1:
will be parsed.
Also I have a very hard time following this function, it's a bit convoluted.
I've tried to come up with something, but it turns out not to be so trivial, so I won't spend more time on this. Here is my attempt with some test cases: https://gist.github.com/boryspoplawski/644b8b6eded440ac259163dca0df0ec9 but as you can see it still fails some cases.
feel free to steal it, if you find it useful
pal/src/host/linux-common/etc_host_info.c line 102 at r4 (raw file):
*/ static bool parse_ip_addr_ipv6(const char** pptr, uint16_t* out_addr) { long part;
Please move to usage
pal/src/host/linux-common/etc_host_info.c line 188 at r4 (raw file):
} /* If we haven't found ':' nor '.' it means it is IPv4 address. */
I think this should be before previous while loop.
pal/src/host/linux-common/etc_host_info.c line 280 at r4 (raw file):
{ "search", resolv_search }, { "options", resolv_options }, { NULL, 0 },
What's the point of this entry? You can loop until i < ARRAY_LEN(resolv_keys).
pal/src/host/linux-common/etc_host_info.c line 287 at r4 (raw file):
const char* startline = buf; while (*ptr != 0x00) {
This loop is weird. Literally every case ends with continue and most of them go to the end of line. Maybe just parse it line by line instead?
pal/src/host/linux-common/etc_host_info.c line 298 at r4 (raw file):
} else if ((ptr != startline) && (*ptr == ' ' || *ptr == '\t')) { for (size_t i = 0; resolv_keys[i].keyword != NULL; i++) { if (strncmp(startline, resolv_keys[i].keyword, ptr - startline - 1) == 0) {
Why this - 1? Is it correct?
pal/src/host/linux-sgx/pal_main.c line 265 at r3 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
Done.
were got -> were taken
pal/src/host/linux-sgx/pal_main.c line 320 at r3 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
Sorry?
Since this function returns an error if it encounters one, you should also return one here, not kill the whole process.
pal/src/host/linux-sgx/pal_main.c line 268 at r4 (raw file):
* - https://www.rfc-editor.org/rfc/rfc1123 * - https://www.ietf.org/rfc/rfc0952.txt * - https://www.rfc-editor.org/rfc/rfc2181
Do we want to have a list of links or just list of RFCs?
pal/src/host/linux-sgx/pal_main.c line 272 at r4 (raw file):
static bool is_hostname_valid(const char* hostname) { const char* ptr = hostname; size_t chrcount = 0;
Maybe label_len instead? I find chrcount not really descriptive.
pal/src/host/linux-sgx/pal_main.c line 281 at r4 (raw file):
('A' <= *ptr && *ptr <= 'Z') || ('0' <= *ptr && *ptr <= '9') || *ptr == '-') {
- Please move the operators to the start of lines (inestead of ends)
- Please ident this with additional 4 spaces. This is the corner case when we do that, because it gets visually mixed with the content of the
{ }block.
pal/src/host/linux-sgx/pal_main.c line 295 at r4 (raw file):
return false; }
I find weird a structure of while loop which always ends with return and uses continue to actually iterate.
Maybe:
Suggestion:
while (ptr - hostname < PAL_HOSTNAME_MAX && *ptr != 0x00) {
if (('a' <= *ptr && *ptr <= 'z')
|| ('A' <= *ptr && *ptr <= 'Z')
|| ('0' <= *ptr && *ptr <= '9')
|| *ptr == '-') {
chrcount++;
} else if (*ptr == '.') {
if (chrcount == 0 || chrcount > 63) {
return false;
}
chrcount = 0;
} else {
return false
}
ptr++;
}pal/src/host/linux-sgx/pal_main.c line 311 at r4 (raw file):
static int import_and_init_emulation_etc_files(struct pal_dns_host_conf* uptr_dns_conf) { struct pal_dns_host_conf* pub_dns = &g_pal_public_state.dns_host; size_t i, j;
Please move to lines where it's used.
pal/src/host/linux-sgx/pal_main.c line 316 at r4 (raw file):
return 0; struct pal_dns_host_conf shallow_dns;
- This is not a shallow copy, it's a flat structure with no nested pointers.
- Why don't you copy over
pub_dnsand validate the content? What's the point of this temporary copy?
pal/src/host/linux-sgx/pal_main.c line 319 at r4 (raw file):
if (!sgx_copy_to_enclave(&shallow_dns, sizeof(shallow_dns), uptr_dns_conf, sizeof(*uptr_dns_conf))) { log_error("Unable to read host info");
host info is way too broad.
Suggestion:
Unable to read host dsn confpal/src/host/linux-sgx/pal_main.c line 348 at r4 (raw file):
for (i = 0, j = 0; i < shallow_dns.dnsrch_count; i++) { if (!is_hostname_valid(shallow_dns.dnsrch[i])) {
Wouldn't this be much easier if we just return an error on invalid hostname? Why do you want to skip them instead?
pal/src/host/linux-sgx/pal_main.c line 742 at r4 (raw file):
} ret = toml_bool_in(g_pal_public_state.manifest_root, "libos.emulate_etc_files", false,
Suggestion:
/*defaultval=*/falsepal/src/host/linux-sgx/pal_main.c line 742 at r4 (raw file):
} ret = toml_bool_in(g_pal_public_state.manifest_root, "libos.emulate_etc_files", false,
This is weird - we only parse this option in PAL, but it's named libos. Why do we even have this g_pal_public_state.emulate_etc_files? Can't we parse this manifest option in LibOS (again, I guess) and remove g_pal_public_state.emulate_etc_files?
pal/src/host/linux-sgx/pal_main.c line 749 at r4 (raw file):
} /* Get host information only for the first process. This information will be
Please be more precise
Code quote:
host informationpal/src/host/linux-sgx/pal_main.c line 752 at r4 (raw file):
* checkpointed and restored during forking of the child process(es). */ if (parent_stream_fd < 0) { if ((ret = import_and_init_emulation_etc_files(uptr_dns_conf)) < 0) {
Can you split this into two lines (ret = and if (ret < 0))? I don't feel like saving one line here with worse readability is better. But not blocking, we do this in many places and we have no official guidelines about it.
There was a problem hiding this comment.
Reviewed 7 of 7 files at r4, all commit messages.
Reviewable status: all files reviewed, 86 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "squash! " found in commit messages' one-liners (waiting on @boryspoplawski, @mkow, and @oshogbo)
a discussion (no related file):
Previously, boryspoplawski (Borys Popławski) wrote…
@dimakuv why the last two files? They were not mentioned before.
Also FWIW this is the list I've found on all servers I had access to (the sum of all seen options):
search,domain,nameserver,options ends0 trust-ad
I just looked at all options that are possible in resolv.conf, and these two options sounded easy to implement and potentially useful. But I agree -- I also haven't seen these options in real-world files.
Documentation/manifest-syntax.rst line 175 at r4 (raw file):
the information gathered from the host. Note that Gramine's parsers for the hosts information support only a subset
the hosts information -> the host's information
pal/src/host/linux-common/etc_host_info.c line 66 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Yup, this code doesn't handle cases like
8.1024==8.0.4.0.
Wow. I didn't know things like 8.1024 are possible for IPv4. Whoever came up with these rules was evil.
pal/src/host/linux-common/etc_host_info.c line 106 at r1 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
We continue, so we do i++ and we back to 0.
Oof. I see. This code is scary, maybe Borys's suggestion would be better?
pal/src/host/linux-common/etc_host_info.c line 130 at r1 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
Yes, that true, but i would prefer to lave this just in case.
Why not add an assert then? That's what asserts are for.
pal/src/host/linux-common/etc_host_info.c line 296 at r1 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
So if I understand correctly, @dimakuv his idea is to thrown an error if the /etc/resolv.conf on the host is empty or that we didn't parse any variable data.
First, I don't think that's a problem - some machines might not have a resolv.conf, and etc emulation might get other useful files (when we will support more of them).
Second, I don't think that's a proper place to do it. For example, we can parse only search keywords, and here we think that we have meaningful data. Later the Pal/SGX-Linux will sanitize it, and decides that domain in search keyword isn't really a domain - this is what I mean by "throw them out".
And we again might end up with empty/etc/resolv.conf. So probably best place would belibos/src/fs/etc/fs.c.But what if this is actually a valid configuration that
/etc/resolv.confis empty?
I agree with Mariuzsh. I'm unblocking.
pal/src/host/linux-common/etc_host_info.c line 37 at r3 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
Done.
@mkow Is this what we want to do? I thought that we use out_ prefix only when we allocate an object inside the function. In other words, we add out_ when we want to indicate that this object should be later deleted via free() or put_...().
But in this function, we simply update the value pointed to by paddr...
pal/src/host/linux-common/etc_host_info.c line 190 at r3 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
With the new name this is longer then 100 lines, not sure how I should split it.
Yes, that's exactly the problem. Maybe a shorter name: parse_values_one_line?
pal/src/host/linux-common/etc_host_info.c line 65 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
What, this comment makes no sense.
8is not converted to8.8.8.8.
Ditto for all other comments.
+1. I guess it should be smth like A[31:24].A[23:16].A[15:8].A[7:0]. Same for other places.
pal/src/host/linux-common/etc_host_info.c line 66 at r4 (raw file):
if (i == 0) { /* Address A has to be converted to: A.A.A.A * The value a is interpreted as a 32-bit. */
a -> A.
pal/src/host/linux-common/etc_host_info.c line 87 at r4 (raw file):
return false; result = result << 8 | addr[i]; }
Personal preference, but I would remove the for loop and just have explicit checks on each of the 4 numbers and an explicit result = addr[0] << 24 | ....
One argument for my formatting is that this code snippet won't rely on result = 0 (which I had to go scroll up and verify).
pal/src/host/linux-sgx/pal_main.c line 320 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Since this function returns an error if it encounters one, you should also return one here, not kill the whole process.
Yes, as Borys said.
pal/src/host/linux-sgx/pal_main.c line 268 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Do we want to have a list of links or just list of RFCs?
List of RFCs is also fine, true. That would be better than links (which may go stale).
pal/src/host/linux-sgx/pal_main.c line 742 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
This is weird - we only parse this option in PAL, but it's named
libos. Why do we even have thisg_pal_public_state.emulate_etc_files? Can't we parse this manifest option in LibOS (again, I guess) and removeg_pal_public_state.emulate_etc_files?
Well, maybe we shouldn't call this option libos? Maybe sys is better? Because we do need to do part of the job in PAL code anyway... @mkow
There was a problem hiding this comment.
Reviewed 7 of 7 files at r4, all commit messages.
Reviewable status: all files reviewed, 63 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "squash! " found in commit messages' one-liners (waiting on @boryspoplawski and @oshogbo)
Documentation/manifest-syntax.rst line 168 at r4 (raw file):
taking the host's configuration via different APIs
different than what? I guess you meant "various"?
libos/src/fs/etc/fs.c line 39 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Could you remove the space from the string and just
+1?
Maybe that would be cleaner only for me, so not blocking
I prefer it inside the string ;)
libos/src/fs/etc/fs.c line 116 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
I think this is just internal name / key to use for
provide_etc_resolv_conffunction, i.g. it's not an object.
It's something like "filesystem id".
pal/include/pal/pal.h line 110 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
I prefer
lengthovercountwhen talking about lists/arrays (becausecountof what, maybe_items_counts...)
As other reviewers didn't mention it I'm not blocking, maybe just personal preference.
I prefer count. What's from with _items_counts interpretation? It's the count of items in the array?
pal/include/pal/pal.h line 39 at r4 (raw file):
/* DNS limits, used in resolv.conf emulation */ #define PAL_MAX_NAMESPACES 3 #define PAL_MAX_DN_SEARCH 6
Or maybe that "DN" means something else?
Suggestion:
PAL_MAX_DNS_SEARCHpal/src/host/linux-common/etc_host_info.c line 66 at r1 (raw file):
Which code is better the committed one or this one:
I think I prefer the committed version.
pal/src/host/linux-common/etc_host_info.c line 296 at r1 (raw file):
thrown an error if the /etc/resolv.conf on the host is empty or that we didn't parse any variable data.
But why merging both cases? Why empty config would be an error?
When we want to fail is after we found some option we recognize, but failed at parsing it.
and decides that domain in search keyword isn't really a domain - this is what I mean by "throw them out".
We should fail then, not silently skip it.
pal/src/host/linux-common/etc_host_info.c line 27 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
I didn't review the usages yet, but looking at other code around, shouldnt we also check for nullbyte?
What do you mean? The current version will stop on a null byte, which seems correct and intended.
pal/src/host/linux-common/etc_host_info.c line 37 at r3 (raw file):
I thought that we use out_ prefix only when we allocate an object inside the function.
Ugh, no? out_ is supposed to mean what its name says, a return value returned via an argument (aka output argument).
pal/src/host/linux-common/etc_host_info.c line 239 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
And that's bad because? Cannot we log unknown options?
But this is not the case for unknown options, but for too long options (which shouldn't appear in normal usage except in cases where the user edited this config file and added some garbage? so, we want to fail loudly).
pal/src/host/linux-common/etc_host_info.c line 65 at r4 (raw file):
uint32_t result = 0; if (i == 0) { /* Address A has to be converted to: A.A.A.A
Suggestion:
Address A has to be converted to A.A.A.Apal/src/host/linux-common/etc_host_info.c line 69 at r4 (raw file):
result = addr[0]; } else if (i == 1) { /* Address A.B has to be converted to: A.B.B.B
Suggestion:
Address A.B has to be converted to A.B.B.Bpal/src/host/linux-common/etc_host_info.c line 76 at r4 (raw file):
result = addr[0] << 24 | addr[1]; } else if (i == 2) { /* Address A.B.C has to be converted to: A.B.C.C
Suggestion:
Address A.B.C has to be converted to A.B.C.Cpal/src/host/linux-sgx/enclave_ecalls.c line 102 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
This will need to be changed when rebasing anyway - now we handle all of this in
pal_linux_main(instead of some random subset here).
This will be a trivial change tho.
@oshogbo: Then why don't you also if dns_conf isn't equal to 1, -1 or 123? ;)
pal/src/host/linux-sgx/host_ecalls.h line 6 at r3 (raw file):
pal_topology.his included bypal.h
Yeah, and so is stdarg.h, but you don't include pal.h to get va_list. It's not part of the public API of pal.h.
pal/src/host/linux-sgx/host_main.c line 632 at r1 (raw file):
we add
out_prefix for those objects that are created inside the function (like strings)
Waait, where did you get that rule from? See another discussion.
pal/src/host/linux-sgx/pal_main.c line 348 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Wouldn't this be much easier if we just return an error on invalid hostname? Why do you want to skip them instead?
+1
There was a problem hiding this comment.
Reviewable status: all files reviewed, 62 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "squash! " found in commit messages' one-liners (waiting on @boryspoplawski, @mkow, and @oshogbo)
libos/src/fs/etc/fs.c line 116 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
It's something like "filesystem id".
Or rather "callback function id"?
pal/include/pal/pal.h line 110 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I prefer count. What's from with
_items_countsinterpretation? It's the count of items in the array?
That was supposed to be _items_count.
count generally reads weirdly to me (for lists, it would make sense for e.g. a set), e.g. I've never seen a count() method on list/array/vector in any language, also I've never seen anybody talking about list count/array count, it's always length.
pal/src/host/linux-common/etc_host_info.c line 27 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
What do you mean? The current version will stop on a null byte, which seems correct and intended.
Idk, brain lag.
pal/src/host/linux-common/etc_host_info.c line 37 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I thought that we use out_ prefix only when we allocate an object inside the function.
Ugh, no?
out_is supposed to mean what its name says, a return value returned via an argument (aka output argument).
@dimakuv is right in the sense that we only added out_ prefix to double-starred arguments, not to output buffers, i.e. when we allocate object inside the function and return it via out-argument, now where we just fill the buffer provided by the caller.
I remember even a (short) discussion about it and managed to find it: https://reviewable.io/reviews/gramineproject/gramine/462#-MzS9EtL831ww-T6HbYg
pal/src/host/linux-sgx/host_main.c line 632 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
we add
out_prefix for those objects that are created inside the function (like strings)Waait, where did you get that rule from? See another discussion.
ditto (it's not really that it wasn't created, but we do not use out_ for output buffers we merely fill)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 62 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "squash! " found in commit messages' one-liners (waiting on @boryspoplawski and @oshogbo)
pal/include/pal/pal.h line 110 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
That was supposed to be
_items_count.
countgenerally reads weirdly to me (for lists, it would make sense for e.g. a set), e.g. I've never seen acount()method on list/array/vector in any language, also I've never seen anybody talking aboutlist count/array count, it's always length.
Dunno, for me count seems more natural, but length is also acceptable, just not "size", which in C/C++ is ambiguous.
pal/src/host/linux-common/etc_host_info.c line 37 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
@dimakuv is right in the sense that we only added
out_prefix to double-starred arguments, not to output buffers, i.e. when we allocate object inside the function and return it via out-argument, now where we just fill the buffer provided by the caller.
I remember even a (short) discussion about it and managed to find it: https://reviewable.io/reviews/gramineproject/gramine/462#-MzS9EtL831ww-T6HbYg
@boryspoplawski: I think you're confusing different cases. Quoting the thread you linked: "AFAIK, by convention we didn't label output buffers as out_, but mostly parameters that are double-star pointers.". This was about cases when an argument is a pointer to an array, not just a single variable passed by a pointer.
See e.g.:
and many more examples, all from the code we recently wrote.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 62 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "squash! " found in commit messages' one-liners (waiting on @boryspoplawski and @oshogbo)
pal/src/host/linux-common/etc_host_info.c line 37 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
@boryspoplawski: I think you're confusing different cases. Quoting the thread you linked: "AFAIK, by convention we didn't label output buffers as out_, but mostly parameters that are double-star pointers.". This was about cases when an argument is a pointer to an array, not just a single variable passed by a pointer.
See e.g.:
and many more examples, all from the code we recently wrote.
Sorry, I thought we were talking about parse_ip_addr_ipv6, where we have uin16_t* out_addr as an output buffer, my bad, sorry for confusion.
But the argument stands there - ipv6 has out_ prefix for buffer output, we can move the discussion to there, or finalize it here. Note that this is most likely different case, from what @dimakuv raised issues about (and I mistakenly mixed them together).
There was a problem hiding this comment.
Reviewable status: all files reviewed, 62 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "squash! " found in commit messages' one-liners (waiting on @boryspoplawski and @oshogbo)
pal/src/host/linux-common/etc_host_info.c line 37 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Sorry, I thought we were talking about
parse_ip_addr_ipv6, where we haveuin16_t* out_addras an output buffer, my bad, sorry for confusion.But the argument stands there - ipv6 has
out_prefix for buffer output, we can move the discussion to there, or finalize it here. Note that this is most likely different case, from what @dimakuv raised issues about (and I mistakenly mixed them together).
Yes, for IPv6 it's a different case, but there I'd still keep the out_ prefix to be more similar to IPv4 function signature (and it's not wrong to use it there, it's just that we don't add out_ for brevity where it's obvious from types/signature - i.e. output buffers).
There was a problem hiding this comment.
Reviewable status: all files reviewed, 62 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "squash! " found in commit messages' one-liners (waiting on @boryspoplawski, @mkow, and @oshogbo)
libos/src/fs/etc/fs.c line 39 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I prefer it inside the string ;)
If we need a tiebreaker, then I also prefer it inside the string (so, as it is currently)
libos/src/fs/etc/fs.c line 116 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Or rather "callback function id"?
Yeah, more like "callback function id".
pal/src/host/linux-common/etc_host_info.c line 37 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Yes, for IPv6 it's a different case, but there I'd still keep the
out_prefix to be more similar to IPv4 function signature (and it's not wrong to use it there, it's just that we don't addout_for brevity where it's obvious from types/signature - i.e. output buffers).
Ok, thanks for clarification. I will follow these rules from now on.
pal/src/host/linux-sgx/host_main.c line 632 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
ditto (it's not really that it wasn't created, but we do not use
out_for output buffers we merely fill)
So looks like we want to have emulated_etc -> out_emulated_etc here :)
Meh, I still find it hard to understand when we use out_ prefix and when we don't force it. Like in this case, it feels both natural (because it is indeed an out argument, i.e., it is modified by this func) and weird to add out_ (because the type bool* already indicates that this will be modified) here. @mkow @boryspoplawski ?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 62 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "squash! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @mkow, and @oshogbo)
pal/src/host/linux-common/etc_host_info.c line 37 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Ok, thanks for clarification. I will follow these rules from now on.
The thing is the function signature is not the same, so making it look the same is misleading. Here we have a single value (uint32_t), there (in ipv6) we have a whole array (of constant len, so no additional parameter passed). Maybe we should juse use uint16_t name[static N] there?
pal/src/host/linux-common/etc_host_info.c line 87 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Personal preference, but I would remove the for loop and just have explicit checks on each of the 4 numbers and an explicit
result = addr[0] << 24 | ....One argument for my formatting is that this code snippet won't rely on
result = 0(which I had to go scroll up and verify).
I had similar thoughts, but the current version was also fine, so didn't comment. Now that I see others think alike, maybe it's better to change it?
pal/src/host/linux-sgx/host_main.c line 632 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
So looks like we want to have
emulated_etc->out_emulated_etchere :)Meh, I still find it hard to understand when we use
out_prefix and when we don't force it. Like in this case, it feels both natural (because it is indeed an out argument, i.e., it is modified by this func) and weird to addout_(because the typebool*already indicates that this will be modified) here. @mkow @boryspoplawski ?
I didn't like this prefix from the beginning and I don't know what were the exact rules you've set when merging such policy.
FWIW, the rules I use are:
*out_name = X;->out_nameshould haveout_prefixmemcpy(name, X, N);/name[i] = X;-> noout_prefix
Note that in the first point it doesn't matter what type X is, i.e. whether it's a pointer to something allocated here or just a scalar value.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 63 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "squash! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @oshogbo)
pal/src/host/linux-common/etc_host_info.c line 37 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
The thing is the function signature is not the same, so making it look the same is misleading. Here we have a single value (
uint32_t), there (in ipv6) we have a whole array (of constant len, so no additional parameter passed). Maybe we should juse useuint16_t name[static N]there?
+1 to [static N] for IPv6
pal/src/host/linux-common/etc_host_info.c line 87 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
I had similar thoughts, but the current version was also fine, so didn't comment. Now that I see others think alike, maybe it's better to change it?
I'm neutral on this.
pal/src/host/linux-sgx/host_main.c line 632 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
I didn't like this prefix from the beginning and I don't know what were the exact rules you've set when merging such policy.
FWIW, the rules I use are:
*out_name = X;->out_nameshould haveout_prefixmemcpy(name, X, N);/name[i] = X;-> noout_prefixNote that in the first point it doesn't matter what type
Xis, i.e. whether it's a pointer to something allocated here or just a scalar value.
@dimakuv: As Borys said. But my general point for keeping this rule is that returning values via arguments is super obscure and we use it only because C is a terrible language. So, I want these places to be marked as visibly as possible.
mkow
changed the title
etc emulation (currently only 'resolv.conf')etc emulation (currently only 'resolv.conf')
There was a problem hiding this comment.
Reviewable status: all files reviewed, 63 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "squash! " found in commit messages' one-liners (waiting on @boryspoplawski, @mkow, and @oshogbo)
pal/src/host/linux-common/etc_host_info.c line 87 at r4 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I'm neutral on this.
Well, it's 2 for the change, 1 abstained. Let's change it then.
pal/src/host/linux-sgx/host_main.c line 632 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
@dimakuv: As Borys said. But my general point for keeping this rule is that returning values via arguments is super obscure and we use it only because C is a terrible language. So, I want these places to be marked as visibly as possible.
Got it. So in this particular case, please replaceemulate_etc -> out_emulate_etc.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 63 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "squash! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @mkow)
Previously, boryspoplawski (Borys Popławski) wrote…
+1, for some reason many people started doing this lately, I wonder why
Done.
Previously, boryspoplawski (Borys Popławski) wrote…
This is wrong.
Hym, I can't fix without force push, so lets this comment just hang here.
Previously, boryspoplawski (Borys Popławski) wrote…
2x sign-off
ditto.
Documentation/manifest-syntax.rst line 160 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
It's not the parsers, it's whole Gramine. Also some potential PALs might not have any parsers at all.
Done.
Documentation/manifest-syntax.rst line 168 at r4 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
taking the host's configuration via different APIs
different than what? I guess you meant "various"?
Done.
Documentation/manifest-syntax.rst line 175 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
the hosts information->the host's information
Done.
libos/src/fs/etc/fs.c line 21 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
+1
Done.
libos/src/fs/etc/fs.c line 100 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
<sincedatamust be null terminated
Done.
libos/src/fs/etc/fs.c line 16 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
What's with these weird indents?
Done.
libos/src/fs/etc/fs.c line 16 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Do we want to keep these newlines here? Looks weird to me... Also other entries add
\nmanually
I think so, it doesn't make sense to add them explicite in all places (in strlen and then when we memcpy).
libos/src/fs/etc/fs.c line 39 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Please change
40to some define, likeMAX_IPV6_ADDR_LENor something.
Done.
libos/src/fs/etc/fs.c line 41 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Let's move this
+1to separate line at the end of this section, so it's clear what it accounts for (a new line, which is after alldnsrchentries). Also IMO it would be good to match the order of accounting to the order we print these chars.
Done.
libos/src/fs/etc/fs.c line 55 at r3 (raw file):
/* Generate data: */ char* ptr = data; int ret = 0;
Done.
pal/include/pal/pal.h line 104 at r3 (raw file):
bool is_ipv6; uint32_t ipv4; uint16_t ipv6[8];
Done.
pal/include/pal/pal.h line 112 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Could we have some meaningful name? I broke my tongue on
dnsrch. Likedns_searchisn't even that long...
Yea sure, I took this name from libresolv.
pal/include/pal/pal.h line 112 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
If you don't indent, then do it everywhere :)
Done.
pal/include/pal/pal.h line 39 at r4 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Or maybe that "DN" means something else?
DN stands for "Domain name" (this is for a search keyword).
pal/src/host/linux/pal_main.c line 422 at r3 (raw file):
} ret = toml_bool_in(g_pal_public_state.manifest_root, "libos.emulate_etc_files", false,
Done.
pal/src/host/linux-common/etc_host_info.c line 130 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why not add an assert then? That's what asserts are for.
I have rewritten this part.
pal/src/host/linux-common/etc_host_info.c line 296 at r1 (raw file):
But why merging both cases? Why empty config would be an error?
I think empty config shouldn't be an error.
We should fail then, not silently skip it.
In case of the invalid hostname we wanted just to set a localhost as a default, we we want to fail on something that potentially user don't have impact on?
pal/src/host/linux-common/etc_host_info.c line 201 at r2 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
%.1337sshould work
How so? We would print the half of host's resolv.conf.
Not sure if we support the asterisk notation?
pal/src/host/linux-common/etc_host_info.c line 7 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Why do you think so?
While looking at the list of supported files proposed in the issue,
we are talking about emulating some files in /etc, so not all of them are network related.
pal/src/host/linux-common/etc_host_info.c line 24 at r3 (raw file):
} static void skip_whitespace(const char** pptr) {
Done.
pal/src/host/linux-common/etc_host_info.c line 33 at r3 (raw file):
} static bool is_end_of_word(const char ch) {
Done.
pal/src/host/linux-common/etc_host_info.c line 37 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
+1 to
[static N]for IPv6
Done.
pal/src/host/linux-common/etc_host_info.c line 42 at r3 (raw file):
char* next; uint32_t addr = 0; int i;
Done.
pal/src/host/linux-common/etc_host_info.c line 88 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Not done
Done, really.
pal/src/host/linux-common/etc_host_info.c line 88 at r3 (raw file):
const char* ptr = *pptr; char* next; int i, twocomas = -1;
Done, really.
pal/src/host/linux-common/etc_host_info.c line 91 at r3 (raw file):
uint16_t addr[8]; memset(&addr, 0, sizeof(addr));
Outdated?
pal/src/host/linux-common/etc_host_info.c line 190 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Yes, that's exactly the problem. Maybe a shorter name:
parse_values_one_line?
Done.
pal/src/host/linux-common/etc_host_info.c line 239 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
But this is not the case for unknown options, but for too long options (which shouldn't appear in normal usage except in cases where the user edited this config file and added some garbage? so, we want to fail loudly).
Fail or report it?
I'm not sure in this situation if we really want to fail. IMHO this might be something that user can't control.
pal/src/host/linux-common/etc_host_info.c line 38 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Please move to usage
Done.
pal/src/host/linux-common/etc_host_info.c line 62 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Please add
assert(i < 4);
Done.
pal/src/host/linux-common/etc_host_info.c line 65 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
+1. I guess it should be smth like
A[31:24].A[23:16].A[15:8].A[7:0]. Same for other places.
Done.
pal/src/host/linux-common/etc_host_info.c line 65 at r4 (raw file):
uint32_t result = 0; if (i == 0) { /* Address A has to be converted to: A.A.A.A
Done.
pal/src/host/linux-common/etc_host_info.c line 66 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
a->A.
Done.
pal/src/host/linux-common/etc_host_info.c line 69 at r4 (raw file):
result = addr[0]; } else if (i == 1) { /* Address A.B has to be converted to: A.B.B.B
Done.
pal/src/host/linux-common/etc_host_info.c line 76 at r4 (raw file):
result = addr[0] << 24 | addr[1]; } else if (i == 2) { /* Address A.B.C has to be converted to: A.B.C.C
Done.
pal/src/host/linux-common/etc_host_info.c line 87 at r4 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I'm neutral on this.
Done.
pal/src/host/linux-common/etc_host_info.c line 101 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
I don't think this function is correct. Things like:
1::\r\r\r+11::0x121::1:will be parsed.
Also I have a very hard time following this function, it's a bit convoluted.I've tried to come up with something, but it turns out not to be so trivial, so I won't spend more time on this. Here is my attempt with some test cases: https://gist.github.com/boryspoplawski/644b8b6eded440ac259163dca0df0ec9 but as you can see it still fails some cases.
feel free to steal it, if you find it useful
I steal parts of it, thanks - Done.
pal/src/host/linux-common/etc_host_info.c line 102 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Please move to usage
Done.
pal/src/host/linux-common/etc_host_info.c line 188 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
I think this should be before previous
whileloop.
Done.
pal/src/host/linux-common/etc_host_info.c line 280 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
What's the point of this entry? You can loop until
i < ARRAY_LEN(resolv_keys).
Done.
pal/src/host/linux-common/etc_host_info.c line 287 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
This loop is weird. Literally every case ends with
continueand most of them go to the end of line. Maybe just parse it line by line instead?
I have rewritten this loop.
pal/src/host/linux-common/etc_host_info.c line 298 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Why this
- 1? Is it correct?
Done.
pal/src/host/linux-sgx/enclave_ecalls.c line 102 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
@oshogbo: Then why don't you also if
dns_confisn't equal to1,-1or123? ;)
Done.
But just out of curiosity: why do we check this in the case of topo?
pal/src/host/linux-sgx/host_ecalls.h line 6 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
pal_topology.his included bypal.hYeah, and so is
stdarg.h, but you don't includepal.hto getva_list. It's not part of the public API ofpal.h.
Ok, gotcha.
pal/src/host/linux-sgx/host_main.c line 632 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
@dimakuv: As Borys said. But my general point for keeping this rule is that returning values via arguments is super obscure and we use it only because C is a terrible language. So, I want these places to be marked as visibly as possible.
Done.
pal/src/host/linux-sgx/host_main.c line 892 at r4 (raw file):
g_host_log_level = log_level; ret = toml_bool_in(manifest_root, "libos.emulate_etc_files", false, emulate_etc);
Done.
pal/src/host/linux-sgx/host_main.c line 895 at r4 (raw file):
if (ret < 0) { log_error("Cannot parse 'libos.emulate_etc_files'"); return ret;
Done.
pal/src/host/linux-sgx/pal_main.c line 265 at r3 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
were got->were taken
Done.
pal/src/host/linux-sgx/pal_main.c line 316 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
No need for this, you literally overwrite it on the next line ;)
Done.
pal/src/host/linux-sgx/pal_main.c line 320 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Yes, as Borys said.
Done.
pal/src/host/linux-sgx/pal_main.c line 268 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
List of RFCs is also fine, true. That would be better than links (which may go stale).
Done.
pal/src/host/linux-sgx/pal_main.c line 272 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Maybe
label_leninstead? I findchrcountnot really descriptive.
Done.
pal/src/host/linux-sgx/pal_main.c line 281 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
- Please move the operators to the start of lines (inestead of ends)
- Please ident this with additional 4 spaces. This is the corner case when we do that, because it gets visually mixed with the content of the
{ }block.
Done.
pal/src/host/linux-sgx/pal_main.c line 295 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
I find weird a structure of while loop which always ends with
returnand usescontinueto actually iterate.
Maybe:
Done.
pal/src/host/linux-sgx/pal_main.c line 311 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Please move to lines where it's used.
Done.
pal/src/host/linux-sgx/pal_main.c line 316 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
- This is not a shallow copy, it's a flat structure with no nested pointers.
- Why don't you copy over
pub_dnsand validate the content? What's the point of this temporary copy?
- Ok.
- It's easier to skip unwanted options. Let's see how the discussion about this will go. If we decide to kill the whole Gramine, I will change that as you suggested.
pal/src/host/linux-sgx/pal_main.c line 319 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
host infois way too broad.
Done.
pal/src/host/linux-sgx/pal_main.c line 348 at r4 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
+1
However, it would want to be so restricted about server configuration, which the user might not have an impact on.
pal/src/host/linux-sgx/pal_main.c line 742 at r4 (raw file):
} ret = toml_bool_in(g_pal_public_state.manifest_root, "libos.emulate_etc_files", false,
Done.
pal/src/host/linux-sgx/pal_main.c line 742 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Well, maybe we shouldn't call this option
libos? Maybesysis better? Because we do need to do part of the job in PAL code anyway... @mkow
I don't have any opinion about this, so waiting on @mkow.
pal/src/host/linux-sgx/pal_main.c line 749 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Please be more precise
Done.
There was a problem hiding this comment.
Reviewed 17 of 17 files at r5, all commit messages.
Reviewable status: all files reviewed, 67 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @oshogbo)
pal/include/host/linux-common/etc_host_info_internal.h line 9 at r5 (raw file):
bool parse_ip_addr_ipv4(const char** pptr, uint32_t* out_addr); bool parse_ip_addr_ipv6(const char** pptr, uint16_t out_addr[8]);
Suggestion:
uint16_t out_addr[static 8]pal/include/pal/pal.h line 39 at r4 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
DN stands for "Domain name" (this is for a
searchkeyword).
Ah, makes sense.
pal/include/pal/pal.h line 104 at r5 (raw file):
struct pal_dns_host_conf_addr { bool is_ipv6;
Please fix alignment
pal/regression/ipv4_parser.c line 13 at r5 (raw file):
#define TEST(x) \ if (x < 0) \ return 1
I'd also move this macro to inside main(), it's only used there and it has a return inside, so better to have it next to where it's used.
Suggestion:
if (x < 0) \
return 1pal/regression/ipv4_parser.c line 43 at r5 (raw file):
if (parse_ip_addr_ipv4(&ptr, &addr)) { pal_printf("We parsed %s successfully, but it is invalid IPv4 address\n", buf);
Suggestion:
it's an invalid IPv4 addresspal/regression/ipv6_parser.c line 13 at r5 (raw file):
#define TEST(x) \ if (x < 0) \ return 1
ditto x2
pal/regression/ipv6_parser.c line 20 at r5 (raw file):
void read_text_file_to_cstr(void) {} static int ipv6_valid(const char* buf, uint16_t reference_addr[8]) {
Suggestion:
uint16_t reference_addr[static 8]pal/regression/ipv6_parser.c line 49 at r5 (raw file):
if (parse_ip_addr_ipv6(&ptr, addr)) { pal_printf("We parsed %s successfully, but it is invalid IPv6 address\n", buf);
ditto
pal/regression/ipv6_parser.c line 72 at r5 (raw file):
{"1337:1:2:3:4:5:6:7", {0x1337, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7}}, {"1337:1:2:3:4:5:6:7 sufix", {0x1337, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7}}, {":: sufix", {0, 0, 0, 0, 0, 0, 0, 0}},
Suggestion:
suffixpal/regression/ipv6_parser.c line 73 at r5 (raw file):
{"1337:1:2:3:4:5:6:7 sufix", {0x1337, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7}}, {":: sufix", {0, 0, 0, 0, 0, 0, 0, 0}}, {"::1 sufix", {0, 0, 0, 0, 0, 0, 0, 1}},
ditto
pal/src/host/linux-common/etc_host_info.c line 296 at r1 (raw file):
In case of the invalid hostname we wanted just to set a localhost as a default, we we want to fail on something that potentially user don't have impact on?
Hmm, but such cases should be rather impossible in real world? (except for hosts which try to attack Gramine) So, I think it's not dangerous to loudly fail, and may actually allow users to notice mistakes easier.
pal/src/host/linux-common/etc_host_info.c line 239 at r3 (raw file):
Fail or report it?
I'd say both. This should never really happen, and worst case users have many possibilities to circumvent this (e.g. by hardcoding resolv.conf in trusted files).
pal/src/host/linux-common/etc_host_info.c line 46 at r5 (raw file):
for (i = 0; i < 4; i++) { /* NOTE: Gramine strtoll/strtol skips white spaces that are before the number, and doesn't * treat this as an error, this behavior is different from glibc.
Could you create an issue about this?
pal/src/host/linux-common/etc_host_info.c line 297 at r5 (raw file):
* From resolv.conf(5): * The keyword and value must appear on a single line, and the * keyword (e.g., nameserver) must start the line. The value
We rather don't use this convention.
Suggestion:
line. Thepal/src/host/linux-common/etc_host_info.c line 306 at r5 (raw file):
ptr += strlen(resolv_keys[i].keyword); /* Because the buffer in strncmp is not ended with 0x00, let's * verify that this is ent of word. */
Suggestion:
end of wordpal/src/host/linux-sgx/enclave_ecalls.c line 102 at r3 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
Done.
But just out of curiosity: why do we check this in the case of topo?
Hmm, probably a bug (or rather just some nonsensical code copypasted from other, older places :) ).
pal/src/host/linux-sgx/pal_main.c line 348 at r4 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
However, it would want to be so restricted about server configuration, which the user might not have an impact on.
If someone's host network config has broken entries then I'd prefer to fail loudly, as it indicates some serious bug in their setup. Normal, "healthy" systems should never trigger this case, right?
pal/src/host/linux-sgx/pal_main.c line 742 at r4 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
I don't have any opinion about this, so waiting on @mkow.
I'd just change it to sys.. I like having that bool explicitly with the data, instead of deriving whether to use the data or not later.
There was a problem hiding this comment.
Reviewed 17 of 17 files at r5, all commit messages.
Reviewable status: all files reviewed, 75 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @mkow, and @oshogbo)
Documentation/manifest-syntax.rst line 175 at r5 (raw file):
the information gathered from the host. Note that Gramine support only a subset of the corresponding options.
supports
pal/regression/ipv4_parser.c line 13 at r5 (raw file):
#define TEST(x) \ if (x < 0) \ return 1
We could simply use CHECK() macro?
pal/regression/ipv4_parser.c line 22 at r5 (raw file):
static int ipv4_valid(const char* buf, uint32_t reference_addr) { const char* ptr = buf; uint32_t ourparser_addr;
For uniformity with the below func, can we have this:
uint32_t addr;
const char* ptr = buf;
In other words, (1) swap the lines, and (2) rename to simply addr -- it is obvious from this function that this is the addr from our parser.
pal/regression/ipv6_parser.c line 13 at r5 (raw file):
#define TEST(x) \ if (x < 0) \ return 1
ditto
pal/regression/ipv6_parser.c line 22 at r5 (raw file):
static int ipv6_valid(const char* buf, uint16_t reference_addr[8]) { const char* ptr = buf; uint16_t ourparser_addr[8];
ditto
pal/regression/ipv6_parser.c line 78 at r5 (raw file):
for (size_t i = 0; i < ARRAY_SIZE(valid_test_cases); i++) { if (ipv6_valid(valid_test_cases[i].str, valid_test_cases[i].addr) < 0) return 1;
Why not use TEST (or as I suggest CHECK) here?
pal/regression/ipv6_parser.c line 102 at r5 (raw file):
TEST(ipv6_invalid("1::\r\r1")); TEST(ipv6_invalid("1:\n:1")); TEST(ipv6_invalid("1::1\r\r:1"));
Please add cases with two (or more) double-colons.
pal/src/host/linux-common/etc_host_info.c line 296 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
In case of the invalid hostname we wanted just to set a localhost as a default, we we want to fail on something that potentially user don't have impact on?
Hmm, but such cases should be rather impossible in real world? (except for hosts which try to attack Gramine) So, I think it's not dangerous to loudly fail, and may actually allow users to notice mistakes easier.
I'm neutral on this. @boryspoplawski ?
pal/src/host/linux-common/etc_host_info.c line 101 at r4 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
I steal parts of it, thanks - Done.
This new version is very readable, thanks!
pal/src/host/linux-common/etc_host_info.c line 49 at r5 (raw file):
*/ if (*ptr == ' ' || *ptr == '\t' || *ptr == '+') return false;
Wouldn't it be better to use isdigit() instead?
pal/src/host/linux-common/etc_host_info.c line 126 at r5 (raw file):
} /* strtol skips 0x prefix, this prefix is invalid in IPv6 */ if (next - ptr >= 2 && !isxdigit(ptr[1]))
Why not move the check for x symbol above, to the not-hex-digit check:
/* note that strtol skips 0x prefix, this prefix is invalid in IPv6 */
if (!isxdigit(ptr[0]) || !isxdigit(ptr[1])) {
return false;
}
pal/src/host/linux-common/etc_host_info.c line 129 at r5 (raw file):
return false; addr[parts_seen] = val; parts_seen++;
In such cases, I prefer: addr[parts_seen++] = val;. Not blocking.
pal/src/host/linux-common/etc_host_info.c line 156 at r5 (raw file):
return false; /* `addr` already correct. */ } else { /* double_colon_pos != -1 */
The comment is useless here :)
pal/src/host/linux-common/etc_host_info.c line 308 at r5 (raw file):
* verify that this is ent of word. */ if (!is_end_of_word(*ptr)) break;
Why do we break here instead of continue? Currently this is correct, but what if we'll have smth like in the future:
resolv_keys[] = {
...
{ "options", resolv_options },
{ "options-more", resolv_options_more },
};
Then with the current break logic, we'll never check options-more
pal/src/host/linux-common/etc_host_info.c line 310 at r5 (raw file):
break; skip_whitespaces(&ptr); resolv_keys[i].set_value(conf, &ptr);
Why did we remove break from here? We'll check against other resolv_keys for no reason...
pal/src/host/linux-common/etc_host_info.c line 317 at r5 (raw file):
jmp_to_end_of_line(&ptr); if (*ptr != 0x00) ptr++;
Can you add an assert before this line that assert(ptr == '\n')? Otherwise I had to scroll up and check what jmp_to_end_of_line() can point to.
pal/src/host/linux-sgx/pal_main.c line 742 at r4 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
Done.
Not done?
pal/src/host/linux-sgx/pal_main.c line 742 at r4 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I'd just change it to
sys.. I like having thatboolexplicitly with the data, instead of deriving whether to use the data or not later.
+1 to Michal.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 75 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @oshogbo)
pal/src/host/linux-common/etc_host_info.c line 156 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
The comment is useless here :)
I get why he did this, but IMO it's helpful only on larger cases (e.g. multiple if-else-if-else-if-else), but even then I prefer an assert for this instead of a comment.
There was a problem hiding this comment.
Reviewable status: 18 of 27 files reviewed, 75 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @mkow, and @oshogbo)
Documentation/manifest-syntax.rst line 175 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
supports
Done.
pal/include/host/linux-common/etc_host_info_internal.h line 9 at r5 (raw file):
bool parse_ip_addr_ipv4(const char** pptr, uint32_t* out_addr); bool parse_ip_addr_ipv6(const char** pptr, uint16_t out_addr[8]);
Done.
pal/include/pal/pal.h line 104 at r5 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Please fix alignment
Done.
pal/regression/ipv4_parser.c line 13 at r5 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I'd also move this macro to inside
main(), it's only used there and it has areturninside, so better to have it next to where it's used.
Done.
pal/regression/ipv4_parser.c line 13 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
We could simply use
CHECK()macro?
Done.
pal/regression/ipv4_parser.c line 22 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
For uniformity with the below func, can we have this:
uint32_t addr; const char* ptr = buf;In other words, (1) swap the lines, and (2) rename to simply
addr-- it is obvious from this function that this is the addr from our parser.
Done.
pal/regression/ipv4_parser.c line 43 at r5 (raw file):
if (parse_ip_addr_ipv4(&ptr, &addr)) { pal_printf("We parsed %s successfully, but it is invalid IPv4 address\n", buf);
Done.
pal/regression/ipv6_parser.c line 13 at r5 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
ditto x2
Done.
pal/regression/ipv6_parser.c line 13 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto
Done.
pal/regression/ipv6_parser.c line 20 at r5 (raw file):
void read_text_file_to_cstr(void) {} static int ipv6_valid(const char* buf, uint16_t reference_addr[8]) {
Done.
pal/regression/ipv6_parser.c line 22 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto
Done.
pal/regression/ipv6_parser.c line 49 at r5 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
ditto
Done.
pal/regression/ipv6_parser.c line 72 at r5 (raw file):
{"1337:1:2:3:4:5:6:7", {0x1337, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7}}, {"1337:1:2:3:4:5:6:7 sufix", {0x1337, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7}}, {":: sufix", {0, 0, 0, 0, 0, 0, 0, 0}},
Done.
pal/regression/ipv6_parser.c line 73 at r5 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
ditto
Done.
pal/regression/ipv6_parser.c line 78 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why not use
TEST(or as I suggestCHECK) here?
Done.
pal/regression/ipv6_parser.c line 102 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please add cases with two (or more) double-colons.
Done.
pal/src/host/linux-common/etc_host_info.c line 239 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Fail or report it?
I'd say both. This should never really happen, and worst case users have many possibilities to circumvent this (e.g. by hardcoding
resolv.confin trusted files).
The issue is that the system ignores everything that is invalid - so user might not even be aware of this.
Dunno I have mixed feelings in any way...
pal/src/host/linux-common/etc_host_info.c line 101 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This new version is very readable, thanks!
Done.
pal/src/host/linux-common/etc_host_info.c line 49 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Wouldn't it be better to use
isdigit()instead?
Good idea.
pal/src/host/linux-common/etc_host_info.c line 126 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why not move the check for
xsymbol above, to the not-hex-digit check:/* note that strtol skips 0x prefix, this prefix is invalid in IPv6 */ if (!isxdigit(ptr[0]) || !isxdigit(ptr[1])) { return false; }
What is the case of 1:2:3...? The second char is not a digit in any case, but it's a valid address.
pal/src/host/linux-common/etc_host_info.c line 129 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
In such cases, I prefer:
addr[parts_seen++] = val;. Not blocking.
I don't like these hidden increments, so if it's not blocking, let's not change that.
pal/src/host/linux-common/etc_host_info.c line 156 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
The comment is useless here :)
Done.
pal/src/host/linux-common/etc_host_info.c line 297 at r5 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
We rather don't use this convention.
Done.
pal/src/host/linux-common/etc_host_info.c line 306 at r5 (raw file):
ptr += strlen(resolv_keys[i].keyword); /* Because the buffer in strncmp is not ended with 0x00, let's * verify that this is ent of word. */
Done.
pal/src/host/linux-common/etc_host_info.c line 308 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why do we break here instead of
continue? Currently this is correct, but what if we'll have smth like in the future:resolv_keys[] = { ... { "options", resolv_options }, { "options-more", resolv_options_more }, };Then with the current
breaklogic, we'll never checkoptions-more
Yep, I wonder about that for a moment, a decision there aren't any known options with the same suffix, so I decided that if found a prefix with known words it's something smelly.
pal/src/host/linux-common/etc_host_info.c line 310 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why did we remove
breakfrom here? We'll check against otherresolv_keysfor no reason...
Yes, we should thanks!
pal/src/host/linux-common/etc_host_info.c line 317 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Can you add an assert before this line that
assert(ptr == '\n')? Otherwise I had to scroll up and check whatjmp_to_end_of_line()can point to.
Done.
pal/src/host/linux-sgx/enclave_ecalls.c line 102 at r3 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Hmm, probably a bug (or rather just some nonsensical code copypasted from other, older places :) ).
ok :) thanks.
pal/src/host/linux-sgx/pal_main.c line 348 at r4 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
If someone's host network config has broken entries then I'd prefer to fail loudly, as it indicates some serious bug in their setup. Normal, "healthy" systems should never trigger this case, right?
Ditto.
pal/src/host/linux-sgx/pal_main.c line 742 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Not done?
Really done.
pal/src/host/linux-sgx/pal_main.c line 742 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
+1 to Michal.
Done.
There was a problem hiding this comment.
Reviewable status: 18 of 27 files reviewed, 75 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @mkow)
pal/src/host/linux-common/etc_host_info.c line 46 at r5 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Could you create an issue about this?
There was a problem hiding this comment.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "squash! " found in commit messages' one-liners
a discussion (no related file):
Previously, oshogbo (Mariusz Zaborski) wrote…
No I have changed it to
extra_runtime_domain_names_conf. Where you seeemulate_etc_files?
Interesting, seems like some caching problem in my editor, refreshing the cache fixed the search results :)
There was a problem hiding this comment.
Reviewed 11 of 11 files at r16, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "squash! " found in commit messages' one-liners (waiting on @oshogbo)
pal/src/host/linux-sgx/pal_main.c line 329 at r16 (raw file):
} static int import_and_init_extra_runtime(struct pal_dns_host_conf* uptr_dns_conf) {
As previously, this is to broad, needs more specification (e.g. include domain names in the name)
pal/src/host/linux-sgx/pal_main.c line 778 at r16 (raw file):
} /* Get host information for extra runtimes configuration only for the first process.
This sentence gives 0 information (after you removed "etc emulation"), what's the point of it
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "squash! " found in commit messages' one-liners (waiting on @boryspoplawski)
pal/src/host/linux-sgx/pal_main.c line 329 at r16 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
As previously, this is to broad, needs more specification (e.g. include
domain namesin the name)
It is broad as I assumed we might want to have one function which take care of all extra runtimes, but I'm not so sure about this.
pal/src/host/linux-sgx/pal_main.c line 778 at r16 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
This sentence gives 0 information (after you removed "etc emulation"), what's the point of it
I assume that the "etc emulation" is know a part of the extra runtime features not a essential key here.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "squash! " found in commit messages' one-liners (waiting on @oshogbo)
pal/src/host/linux-sgx/pal_main.c line 329 at r16 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
It is broad as I assumed we might want to have one function which take care of all extra runtimes, but I'm not so sure about this.
But what runtimes? We have nothing atm and no plans (at least not formulated concretely) of adding anything more (than network/dn conf).
Besides, this sounds like you would be importing some language runtime, whatever that would mean.
pal/src/host/linux-sgx/pal_main.c line 778 at r16 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
I assume that the "etc emulation" is know a part of the extra runtime features not a essential key here.
But this sentence is basically "we do stuff" i.e. it doesn't explain anything.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "squash! " found in commit messages' one-liners (waiting on @boryspoplawski)
pal/src/host/linux-sgx/pal_main.c line 329 at r16 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
But what runtimes? We have nothing atm and no plans (at least not formulated concretely) of adding anything more (than network/dn conf).
Besides, this sounds like you would be importing some language runtime, whatever that would mean.
Done.
pal/src/host/linux-sgx/pal_main.c line 778 at r16 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
But this sentence is basically "we do stuff" i.e. it doesn't explain anything.
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r17, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners
There was a problem hiding this comment.
Reviewed 10 of 11 files at r16, 1 of 1 files at r17, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners
There was a problem hiding this comment.
Reviewed 1 of 1 files at r17, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners
oshogbo
dismissed stale reviews from mkow, dimakuv, and boryspoplawski
via
f6f7f5c
oshogbo
force-pushed
the
oshogbo/etc_resolv
branch
from
ef5659d
to
f6f7f5c
Compare
There was a problem hiding this comment.
Reviewed 2 of 2 files at r18, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL)
There was a problem hiding this comment.
Reviewed 2 of 2 files at r18, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @oshogbo)
- This one liner is too long
/etc/resolv.conf
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dimakuv)
Previously, boryspoplawski (Borys Popławski) wrote…
- This one liner is too long
/etc/resolv.conf
This information in first line was requested by @dimakuv, and I don't think we can fit all this information without breaking line length.
oshogbo
force-pushed
the
oshogbo/etc_resolv
branch
from
f6f7f5c
to
e6c5357
Compare
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv)
Previously, oshogbo (Mariusz Zaborski) wrote…
This information in first line was requested by @dimakuv, and I don't think we can fit all this information without breaking line length.
Done.
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @boryspoplawski)
Previously, oshogbo (Mariusz Zaborski) wrote…
Done.
@oshogbo Let's remove (currently only 'resolv.conf') phrase. And add the sentence to the commit message at the end, smth like this:
... a pseudo file (using pseudofs filesystem). Currently only
`/etc/resolv.conf` is introduced.
UPDATE: Mariusz updated before I finished this comment. I guess I'm fine with this version too.
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
@oshogbo Let's remove
(currently only 'resolv.conf')phrase. And add the sentence to the commit message at the end, smth like this:... a pseudo file (using pseudofs filesystem). Currently only `/etc/resolv.conf` is introduced.UPDATE: Mariusz updated before I finished this comment. I guess I'm fine with this version too.
I would prefer to add the sentence mentioned by @dimakuv but not blocking.
To accomplish this, Gramine obtains information from the host, sanitizes it, and stores it in the global PAL state. Later, LibOS uses it to create a pseudo file (using pseudofs filesystem). Currently only `/etc/resolv.conf` is introduced. Signed-off-by: Mariusz Zaborski <[email protected]>
oshogbo
dismissed stale reviews from boryspoplawski and dimakuv
via
07fb016
oshogbo
force-pushed
the
oshogbo/etc_resolv
branch
from
e6c5357
to
07fb016
Compare
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL)
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from different teams (1 more required, approved so far: ITL)
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status:
Description of the changes
The first
etcpassthrough file is the/etc/resolv.conf. Gramine reads the option to global PAL state, and LibOS uses it to create a pseudo file with its content.In case of
nameserverwe support only IPv4 in canonical form and with skipped zeros (eg. 8.8, 8, 8.8.8), and IPv6 in abbreviations (::1,ff::1:2) and full IP addresses.In case of
searchwe expects a valid hostname.Issue
#689
Next step
hostnameHow to test this PR?
Check if
/etc/resolv.confis exists with the valid content.This change is