View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0009295 | GNUnet | util library | public | 2024-10-23 14:28 | 2024-11-14 09:43 |
| Reporter | fefe | Assigned To | Christian Grothoff | ||
| Priority | normal | Severity | minor | Reproducibility | have not tried |
| Status | assigned | Resolution | open | ||
| Target Version | 0.23.0 | ||||
| Summary | 0009295: GNUNET_DISK_file_test is inherently racy | ||||
| Description | This function is called, for example, from GNUNET_CONFIGURATION_default. It will call stat() on the filename and then access. The access man page explicitly warns of using it because that would be a race condition. One of the checks is to make sure it's a regular file. Here is the code from GNUNET_CONFIGURATION_default: 2501 /* First, try user configuration. */ 2502 if (NULL != xdg) 2503 GNUNET_asprintf (&cfgname, 2504 "%s/%s", 2505 xdg, 2506 dpd->config_file); 2507 else 2508 cfgname = GNUNET_strdup (dpd->user_config_file); 2509 2510 /* If user config doesn't exist, try in 2511 /etc/<projdir>/<cfgfile> and /etc/<cfgfile> */ 2512 if (GNUNET_OK != GNUNET_DISK_file_test (cfgname)) 2513 { 2514 GNUNET_free (cfgname); 2515 GNUNET_asprintf (&cfgname, 2516 "/etc/%s", 2517 dpd->config_file); 2518 } 2519 if (GNUNET_OK != GNUNET_DISK_file_test (cfgname)) 2520 { 2521 GNUNET_free (cfgname); 2522 GNUNET_asprintf (&cfgname, 2523 "/etc/%s/%s", 2524 dpd->project_dirname, 2525 dpd->config_file); 2526 } 2527 if (GNUNET_OK != GNUNET_DISK_file_test (cfgname)) 2528 { 2529 LOG (GNUNET_ERROR_TYPE_ERROR, 2530 "Unable to top-level configuration file.\n"); 2531 GNUNET_OS_init (pd); 2532 GNUNET_CONFIGURATION_destroy (cfg); 2533 GNUNET_free (cfgname); 2534 return NULL; 2535 } Note that this does the same stat test multiple times. Also all the stat tests are worthless if the filesystem changes between the access and the open. Here is the open: 2537 /* We found a configuration file that looks good, try to load it. */ 2538 2539 LOG (GNUNET_ERROR_TYPE_DEBUG, 2540 "Loading top-level configuration from '%s'\n", 2541 cfgname); 2542 if (GNUNET_OK != 2543 GNUNET_CONFIGURATION_load (cfg, cfgname)) The preferred method is to call open directly and then use fstat to check we opened a regular file. | ||||
| Tags | No tags attached. | ||||
|
|
Yeah we obviously call stat way too often. And fstat is missing from the open API. |
|
|
Ok I took a jab at this. GNUNET_DISK_file_test is not inherently racy. It is just unreliable when used in combination with open() like this. It also has other uses. GNUNET_CONFIGURATION_parse uses GNUNET_DISK_fn_read which does not return a file descriptor and can be used with non-regular files as well (e.g. device files). So, this would require rewriting GNUNET_CONFIGURATION_parse to use another API. We may want to check all uses of GNUNET_DISK_fn_read, but I don't think this is a critical bug. Leaving this open for now. |
|
|
What I was trying to say is: A function that calls stat and access is worthless. You might as well always return size = 23 and GNUNET_OK. Between the call to the function and when you actually use the values, the state of the filesystem could have changed. In particular someone could symlink-race you to trick you into opening a device instead of a regular file. You are clearly trying to prevent that by having this check. I'm telling you that the check is basically a NOP. If you want to prevent accidentally opening /dev/zero, then do the open first and then use fstat. |
|
|
Yes, I understand. I am not convinced that the goal is/was to prevent this. the file_test calls are just there to bail out early, for example if the file does not exist/is unreadable. If we eventually open /dev/zero then the code will properly handle this as part of the parse. In other words, the code does not require the configuration file to be a regular file. And I am not sure that is a requirement. If it is, then we should add a new API that allows us to check if the opened file is a regular file, and not use GNUNET_DISK_fn_read but GNUNET_DISK_file_open to use the file handle on that new API. |
|
|
@fefe: I just happened to put another use of that API into libgnunetpq. There, I check of a file exchange-${N}.sql exists, if so, I fork+exec psql on it and move on to exchange-${N+1}.sql. No point in atomic operations here, but I need to know when to stop my for()-loop, and I shouldn't just call psql on a file that doesn't even exist (both for performance reasons, and because I need a good way to distinguish the two main outcomes: psql failed to parse the file/run on it, and "loop finished"; and psql doesn't give me an easy way to tell file-not-found from syntax-error). Note that this is on a directory in /usr which is basically read-only (root-owned, application never writes there), so there is also no real problem with races either. |
|
|
As far as configuration.c is concerned, yes, those could be replaced by a direct open() attempt of the file (and then fstat()). That said, the configuration file structure is also not considered to be something where we'd worry about races on disk, and here we're merely trying out possible names where configuration data may be located. |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2024-10-23 14:28 | fefe | New Issue | |
| 2024-10-23 20:36 | schanzen | Note Added: 0023576 | |
| 2024-10-23 21:12 | schanzen | Note Added: 0023577 | |
| 2024-10-23 21:12 | schanzen | Note Edited: 0023577 | |
| 2024-10-24 12:40 | fefe | Note Added: 0023589 | |
| 2024-10-24 13:15 | schanzen | Note Added: 0023590 | |
| 2024-10-24 13:28 | schanzen | Target Version | => 0.22.2 |
| 2024-10-24 13:28 | schanzen | Assigned To | => schanzen |
| 2024-10-24 13:28 | schanzen | Status | new => assigned |
| 2024-10-29 13:18 | schanzen | Assigned To | schanzen => Christian Grothoff |
| 2024-10-29 13:18 | schanzen | Target Version | 0.22.2 => 0.22.3 |
| 2024-11-10 17:57 | Christian Grothoff | Note Added: 0023684 | |
| 2024-11-10 18:02 | Christian Grothoff | Note Added: 0023685 | |
| 2024-11-14 09:43 | schanzen | Target Version | 0.22.3 => 0.23.0 |
