Skip to content

refactor(system): move locale env collection to system package for child process reuse#419

Merged
zhaohuiw42 merged 1 commit into
linuxdeepin:masterfrom
zhaohuiw42:p-fix-361139
May 14, 2026
Merged

refactor(system): move locale env collection to system package for child process reuse#419
zhaohuiw42 merged 1 commit into
linuxdeepin:masterfrom
zhaohuiw42:p-fix-361139

Conversation

@zhaohuiw42

Copy link
Copy Markdown
Contributor

Export CollectAndClearLocaleEnvs and OriginalLocaleEnvs to allow child processes (e.g. apt commands) to inherit locale environment variables from the daemon process.

将语言环境变量收集函数迁移至 system 包并导出,使子进程(如 apt 命令)
能够从守护进程继承语言环境变量。

Log: 将语言环境变量收集函数迁移至 system 包
PMS: BUG-361139
Influence: 子进程执行 apt 命令时可正确继承语言环境变量,确保输出信息语言一致。

…ild process reuse

Export CollectAndClearLocaleEnvs and OriginalLocaleEnvs to allow child
processes (e.g. apt commands) to inherit locale environment variables
from the daemon process.

将语言环境变量收集函数迁移至 system 包并导出,使子进程(如 apt 命令)
能够从守护进程继承语言环境变量。

Log: 将语言环境变量收集函数迁移至 system 包
PMS: BUG-361139
Influence: 子进程执行 apt 命令时可正确继承语言环境变量,确保输出信息语言一致。
@github-actions

github-actions Bot commented May 14, 2026

Copy link
Copy Markdown

CLA Assistant Lite bot:
提交邮箱中包含我们的合作伙伴,但您似乎并非合作伙伴的成员或对接人,请联系相关对接人将您添加至组织之中,或由其重新发起 Pull Request。
The commit email domain belongs to one of our partners, but it seems you are not yet a member of the current organization, please contact the contact person to add you to the organization or let them submit the Pull Request.

You can retrigger this bot by commenting recheck in this Pull Request

@deepin-ci-robot

Copy link
Copy Markdown

deepin pr auto review

这份代码变更的主要目的是将收集和清理本地化环境变量的逻辑从 lastore-daemon 包下沉到 system 包中,使其成为公共函数和变量,以便在不同的模块(如 apt/proxy.goimmutable.go)中复用。

整体来看,代码逻辑清晰,测试覆盖也较为完善。但从语法逻辑、代码质量、性能和安全性角度,仍有以下改进建议:

1. 语法与逻辑

1.1 环境变量注入方式不一致
immutable.go 中,环境变量是通过 append 追加到 cmd.Env 切片的:

cmd.Env = append(os.Environ(), "IMMUTABLE_DISABLE_REMOUNT=false")
cmd.Env = append(cmd.Env, system.OriginalLocaleEnvs...)

而在 apt/proxy.go 中,环境变量是通过 Map 注入的:

environ := map[string]string{...}
for _, env := range system.OriginalLocaleEnvs {
    parts := strings.SplitN(env, "=", 2)
    environ[parts[0]] = parts[1]
}

问题OriginalLocaleEnvs 存储的是 []string(如 "LANG=zh_CN.UTF-8"),在 apt/proxy.go 中又需要将其拆解为 Map,而在 immutable.go 中直接作为 cmd.Env 使用。这种“先拼接再拆解”的设计增加了不必要的解析开销,且容易因格式不符预期导致逻辑错误。

建议:统一环境变量的存储和传递结构。建议将 OriginalLocaleEnvs 改为 map[string]string,或者定义一个专门的结构体。这样在 apt/proxy.go 中可以直接遍历赋值,在 immutable.go 中再根据需要拼接成 []string 传给 cmd.Env

2. 代码质量

2.1 全局变量的并发安全性
system.OriginalLocaleEnvs 是一个包级别的全局变量,且在 CollectAndClearLocaleEnvs 中被重新赋值。
问题:如果 CollectAndClearLocaleEnvs 在程序运行期间被并发调用,或者在其他 goroutine 正在遍历 OriginalLocaleEnvs 时发生重赋值,会引发数据竞争。
建议

  • 如果该函数只在程序启动时调用一次(从 main.go 看极大概率是这样),建议在注释中明确标注其初始化阶段调用的约束:// Must be called only once during initialization.
  • 如果存在并发场景,应使用 sync.RWMutex 保护对该变量的读写,或者使用 sync.Once 确保只执行一次。

2.2 错误处理被忽略
CollectAndClearLocaleEnvs 中:

_ = utils.UnsetEnv(localeEnvName)

问题:虽然清理环境变量失败通常不影响主流程,但完全忽略错误可能会在极端情况下掩盖系统环境异常。
建议:如果 utils.UnsetEnv 可能返回需要关注的错误,应至少记录一条警告日志;如果它设计上就是可能失败且无需关注的,当前的 _ 赋值是可以接受的,但建议加一行简短注释说明原因。

3. 代码性能

3.1 字符串拆分与解析开销
如前所述,apt/proxy.go 中的 strings.SplitN 带来了额外的内存分配和计算开销。
优化建议:如果将 OriginalLocaleEnvs 设计为 map[string]stringapt/proxy.go 中的代码可以简化为:

for k, v := range system.OriginalLocaleEnvs {
    environ[k] = v
}

不仅代码更简洁,性能也更好。

4. 代码安全

4.1 环境变量注入风险
immutable.go 中:

cmd.Env = append(os.Environ(), "IMMUTABLE_DISABLE_REMOUNT=false")
cmd.Env = append(cmd.Env, system.OriginalLocaleEnvs...)

问题:如果 os.Environ() 中已经包含了 LANG 等变量,append 操作会把 OriginalLocaleEnvs 追加到末尾。在 POSIX 标准中,当环境变量重名时,通常取第一次出现的值。这意味着追加在末尾的 LANG 可能不会生效,反而 os.Environ() 中原有的(可能已被主进程清除的)变量会生效。
分析:根据代码逻辑,主进程在 main.go 中调用了 CollectAndClearLocaleEnvs 清除了自身的环境变量,因此 os.Environ() 中理论上不应该再包含这些 Locale 变量。但为了防范逻辑漏洞或执行时序问题,更安全的做法是构建一个纯净的、去重后的环境变量列表。

建议:如果 OriginalLocaleEnvs 改为 Map,在构建 cmd.Env 时,可以遍历 os.Environ(),跳过已存在于 Map 中的键,然后再将 Map 中的变量追加进去,确保子进程继承的 Locale 环境变量完全受控。

综合改进后的代码示例

基于以上建议,以下是重构后的代码片段:

src/internal/system/common.go

// OriginalLocaleEnvs stores the locale environment variables collected at startup.
// It is recommended to use sync.RWMutex if concurrent access is possible.
var OriginalLocaleEnvs map[string]string

// CollectAndClearLocaleEnvs stores current locale envs for child processes that
// still need them, then clears them from the daemon process environment.
// Note: This function should only be called once during initialization.
func CollectAndClearLocaleEnvs() {
	OriginalLocaleEnvs = make(map[string]string)
	for _, localeEnvName := range []string{"LC_ALL", "LANGUAGE", "LC_MESSAGES", "LANG"} {
		localeEnvValue, exists := os.LookupEnv(localeEnvName)
		if exists {
			OriginalLocaleEnvs[localeEnvName] = localeEnvValue
		}
		// Ignore error as clearing env is non-critical for daemon execution
		_ = utils.UnsetEnv(localeEnvName) 
	}
}

// EnvsAsSlice returns the locale envs in KEY=VALUE format for exec.Cmd.Env
func EnvsAsSlice() []string {
	envs := make([]string, 0, len(OriginalLocaleEnvs))
	for k, v := range OriginalLocaleEnvs {
		envs = append(envs, k+"="+v)
	}
	return envs
}

src/internal/system/apt/proxy.go

environ := map[string]string{
	"IMMUTABLE_DISABLE_REMOUNT": "false",
}
// 直接遍历 Map,无需 SplitN
for k, v := range system.OriginalLocaleEnvs {
	environ[k] = v
}

src/lastore-daemon/immutable.go

if system.NormalFileExists(system.DeepinImmutableCtlPath) {
	cmd := exec.Command(system.DeepinImmutableCtlPath, args...) // #nosec G204
	// 构建安全且不重复的子进程环境变量
	cmd.Env = append(os.Environ(), "IMMUTABLE_DISABLE_REMOUNT=false")
	cmd.Env = append(cmd.Env, system.EnvsAsSlice()...)
    // ... 后续逻辑
}

src/internal/system/common_test.go (相应调整断言)

assert.Equal(t, map[string]string{
	"LC_ALL":     "zh_CN.UTF-8",
	"LANGUAGE":   "zh_CN:en_US",
	"LC_MESSAGES":"zh_CN.UTF-8",
	"LANG":       "zh_CN.UTF-8",
}, OriginalLocaleEnvs)

通过以上改进,代码的数据结构更加合理,避免了不必要的字符串解析,增强了环境变量注入的安全性,并在注释中明确了并发使用的约束。

@deepin-ci-robot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: electricface, zhaohuiw42

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@zhaohuiw42 zhaohuiw42 merged commit 585f355 into linuxdeepin:master May 14, 2026
13 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants