View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0009274 | GNUnet | other | public | 2024-10-17 13:48 | 2024-10-29 20:56 |
| Reporter | fefe | Assigned To | schanzen | ||
| Priority | normal | Severity | minor | Reproducibility | have not tried |
| Status | closed | Resolution | fixed | ||
| Target Version | 0.22.2 | Fixed in Version | 0.22.2 | ||
| Summary | 0009274: _make_continuous_arg_copy (in gnunet): integer overflow | ||||
| Description | To be honest, I don't quite understand why you need this function in the first place. You already have a perfectly good copy of argv which you don't need to free at all. Also, the way you allocate the new struct means it is basically read-only as you can't resize any of the strings. If you weren't going to write, why make a copy to begin with? Still, the code also has an integer overflow bug: 1231 for (int i = 0; i < argc; i++) 1232 { 1233 size_t ail = strlen (argv[i]); 1234 1235 GNUNET_assert (SIZE_MAX - 1 - sizeof (char *) > argvsize); 1236 GNUNET_assert (SIZE_MAX - ail > argvsize + 1 + sizeof (char*)); 1237 argvsize += strlen (argv[i]) + 1 + sizeof(char *); 1238 } The assertions prevent integer overflows when counting one pointer and one zero terminated string per element, but you still need to add one char* for the final NULL sentinel: 1239 new_argv = GNUNET_malloc (argvsize + sizeof(char *)); Nobody has checked that this does not overflow. It probably won't, but the same can be said for the previous checks. The kernel has a size limit for argv, I think it's 64k or so. There is one other potential optimization: 1241 for (int i = 0; i < argc; i++) 1242 { 1243 new_argv[i] = p; 1244 strcpy (p, argv[i]); 1245 p += strlen (argv[i]) + 1; 1246 } If you use stpcpy instead of strcpy, you don't have to run strlen again. stpcpy used to be a non-standard extension but has been part of POSIX for a while now. | ||||
| Tags | No tags attached. | ||||
|
|
My guess from the API documentation is that when we supported Windows natively, this command converted the non-UTF-8 arguments to UTF-8 arguments in the newly allocated buffer. Since we dropped W32 support some time ago, this function should probably be a NOP. |
|
|
https://git.gnunet.org/gnunet.git/commit/ |
|
|
Eh, it's not just W32 where the command-line may not be in UTF-8! i think this is a very bad fix, as it'll really badly break things if a command-line is not using UTF-8, and as that is rare, it'll be a very bad surprise. |
|
|
The code did nothing and was documented as such. If you want to add a new feature/API that actually does something, feel free to do so. |
|
|
Here you can see the w32 code that was remove some time ago from this function: https://git.gnunet.org/gnunet.git/tree/src/util/strings.c?id=ec472b1aae122481f4f7e760e5242753eba9bf87#n1524 And without the define you cannot really just "encode to UTF-8". Because for that, you'd need to source encoding. And that may be anything. So the API is not suitable for this problem anyway. For windows it made sense, because the source encoding is/was known. For *nix, we always assumed UTF-8, and I think we should continue to do so, and that makes this function superfluous. |
|
|
Ok. |
|
|
Released |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2024-10-17 13:48 | fefe | New Issue | |
| 2024-10-17 13:49 | fefe | Summary | _make_continuous_arg_copy: integer overflow => _make_continuous_arg_copy (in gnunet): integer overflow |
| 2024-10-17 13:50 | fefe | Project | Taler => GNUnet |
| 2024-10-23 13:20 | schanzen | Target Version | git (master) => 0.22.2 |
| 2024-10-23 21:45 | schanzen | Note Added: 0023581 | |
| 2024-10-24 12:31 | schanzen | Assigned To | => schanzen |
| 2024-10-24 12:31 | schanzen | Status | new => resolved |
| 2024-10-24 12:31 | schanzen | Resolution | open => fixed |
| 2024-10-24 12:31 | schanzen | Fixed in Version | => 0.22.2 |
| 2024-10-24 12:31 | schanzen | Note Added: 0023587 | |
| 2024-10-24 18:32 | Christian Grothoff | Note Added: 0023592 | |
| 2024-10-24 20:12 | schanzen | Note Added: 0023593 | |
| 2024-10-25 11:46 | schanzen | Note Added: 0023595 | |
| 2024-10-25 16:32 | Christian Grothoff | Note Added: 0023604 | |
| 2024-10-29 20:56 | schanzen | Note Added: 0023626 | |
| 2024-10-29 20:56 | schanzen | Status | resolved => closed |
