bryancall commented on code in PR #12717:
URL: https://github.com/apache/trafficserver/pull/12717#discussion_r2879997008
##########
src/iocore/cache/CacheHosting.cc:
##########
@@ -740,17 +743,33 @@ ConfigVolumes::BuildListFromString(char
*config_file_path, char *file_buf)
in_percent = 0;
}
} else if (strcasecmp(tmp, "avg_obj_size") == 0) { // match avg_obj_size
- tmp += 13;
- avg_obj_size = atoi(tmp);
+ tmp += 13;
+ if (!ParseRules::is_digit(*tmp)) {
+ err = "Invalid avg_obj_size value (must start with a number, e.g.,
64K)";
+ break;
+ }
+ avg_obj_size = static_cast<int>(ink_atoi64(tmp));
- while (ParseRules::is_digit(*tmp)) {
+ if (avg_obj_size < 0) {
+ err = "Invalid avg_obj_size value (must be >= 0)";
+ break;
+ }
Review Comment:
`ink_atoi64` only recognizes uppercase suffixes (`K`, `M`, `G`, `T`), but
this skip loop accepts both cases via `strchr("kmgtKMGT", *tmp)`. If someone
writes `ram_cache_size=2g`, `ink_atoi64` returns `2` (no multiplier applied),
the skip loop eats the `g`, and the volume silently gets a 2-byte RAM cache
allocation.
Either restrict the skip to uppercase only:
```cpp
while (*tmp && (ParseRules::is_digit(*tmp) || strchr("KMGT", *tmp))) {
```
Or better, reject lowercase with an error. Same issue applies to
`ram_cache_cutoff`, `avg_obj_size`, and `fragment_size` parsing.
##########
src/proxy/http/remap/RemapConfig.cc:
##########
@@ -1197,12 +1209,55 @@ remap_parse_config_bti(const char *path,
BUILD_TABLE_INFO *bti)
if ((bti->remap_optflg & REMAP_OPTFLG_MAP_ID) != 0) {
int idx = 0;
int ret = remap_check_option(bti->argv, bti->argc, REMAP_OPTFLG_MAP_ID,
&idx);
+
if (ret & REMAP_OPTFLG_MAP_ID) {
- char *c = strchr(bti->argv[idx], static_cast<int>('='));
+ char *c = strchr(bti->argv[idx], static_cast<int>('='));
+
new_mapping->map_id = static_cast<unsigned int>(atoi(++c));
}
}
+ // Parse @volume= option with comma-separated syntax (@volume=3,4)
+ for (int i = 0; i < bti->argc; i++) {
+ if (!strncasecmp(bti->argv[i], "volume=", 7)) {
+ const char *volume_str = &bti->argv[i][7];
+
+ if (!volume_str || !*volume_str) {
+ snprintf(errStrBuf, sizeof(errStrBuf), "Empty @volume= directive at
line %d", cln + 1);
+ errStr = errStrBuf;
+ goto MAP_ERROR;
+ }
+
+ for (const char *p = volume_str; *p; p++) {
+ if (*p != ',' && (*p < '0' || *p > '9')) {
+ snprintf(errStrBuf, sizeof(errStrBuf), "Invalid character '%c' in
@volume=%s at line %d", *p, volume_str, cln + 1);
+ errStr = errStrBuf;
Review Comment:
This character-level validation allows degenerate inputs: `@volume=,,`,
`@volume=0`, `@volume=,1,`, `@volume=999`. For the eager path (cache ready),
`createCacheHostRecord()` catches bad volume numbers. But for the deferred path
(initial startup), the string is stored as-is and errors only surface later as
a non-fatal `Error()` log — the remap rule silently runs without a volume
override.
Consider adding lightweight range validation here (split on commas, verify
each number is 1–255, reject empty segments).
##########
src/proxy/ReverseProxy.cc:
##########
@@ -168,6 +167,65 @@ reloadUrlRewrite()
}
}
+/**
+ * Helper function to initialize volume_host_rec for a single url_mapping.
+ * This is a no-op if the mapping has no volume string or is already
initialized.
+ */
+static void
+init_mapping_volume_host_rec(url_mapping &mapping)
+{
+ char errbuf[256];
+
+ if (!mapping.initVolumeHostRec(errbuf, sizeof(errbuf))) {
+ Error("Failed to initialize volume record for @volume=%s: %s",
mapping.getVolume().c_str(), errbuf);
+ }
+}
+
+static void
+init_store_volume_host_records(UrlRewrite::MappingsStore &store)
+{
+ if (store.hash_lookup) {
+ for (auto &entry : *store.hash_lookup) {
+ UrlMappingPathIndex *path_index = entry.second;
+
+ if (path_index) {
+ path_index->foreach_mapping(init_mapping_volume_host_rec);
+ }
+ }
+ }
+
+ for (UrlRewrite::RegexMapping *reg_map = store.regex_list.head; reg_map;
reg_map = reg_map->link.next) {
+ if (reg_map->url_map) {
+ init_mapping_volume_host_rec(*reg_map->url_map);
+ }
+ }
+}
+
+// This is called after the cache is initialized, since we may need the
volume_host_records.
+// Must only be called during startup before any remap reload can occur.
+void
+init_remap_volume_host_records()
+{
+ UrlRewrite *table = rewrite_table;
Review Comment:
Minor: `UrlRewrite *table = rewrite_table;` is a non-atomic read of a
pointer that `reloadUrlRewrite()` modifies via `ink_atomic_swap`. The
startup-only constraint makes this safe in practice, but an atomic load would
make the intent explicit:
```cpp
UrlRewrite *table = ink_atomic_load(&rewrite_table);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
