Page MenuHomeFreeBSD

lib/libc: Fix uninitialized value in __setenv()
Needs ReviewPublic

Authored by arichardson on Aug 2 2021, 2:42 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Apr 11, 7:40 PM
Unknown Object (File)
Feb 7 2024, 2:32 PM
Unknown Object (File)
Dec 20 2023, 6:52 AM
Unknown Object (File)
Oct 24 2023, 8:31 PM
Unknown Object (File)
Aug 31 2023, 2:56 PM
Unknown Object (File)
Jul 13 2023, 8:35 AM
Unknown Object (File)
Jul 8 2023, 2:19 PM
Unknown Object (File)
Jul 4 2023, 1:11 AM

Details

Reviewers
avg
emaste
kib
oshogbo
Group Reviewers
secteam
Summary

Reported indirectly via MK_UBSAN:
lib/libc/stdlib/getenv.c:169:20: runtime error: load of value 190, which is not a valid value for type 'bool'

MFC after: 3 days

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 40805
Build 37694: arc lint + arc unit

Event Timeline

This revision is now accepted and ready to land.Sep 13 2021, 10:21 AM
This revision now requires review to proceed.Sep 13 2021, 10:21 AM
oshogbo added a subscriber: oshogbo.

If I understand correctly, the envVars with the putenv can't be reused.
So the putenv actually is set to 0 (as we are doing calloc'ing the memory needed for envVars), and we are 0 memory when we needed.
So besides the unhappy MK_UBSAN, there are no functional changes, as well as there is no security implication.
If I'm wrong at some of these statements, please correct me.

This revision is now accepted and ready to land.Sep 13 2021, 6:57 PM

Well ubsan is complaining that it loaded a value of 190 so that memory clearly wasn't zeroed.

I have some reservation with the proposed change. Basically the existing code is assuming that unused environment slots are zeroed (and they should be), so if I was you I'd probably amend the code around the two reallocarray's by zeroing out the newly added memory instead.

Oh. Right.
I didn't noticed that after realloc the memory is not cleaned.
Yes, I also would go with @delphij suggestion.

Sounds good, happy to zero after realloc instead. Will update soon.

lib/libc/stdlib/getenv.c
283

environSize * sizeof(*intEnviron) is the common subexpression in both src and size. I suspect that extracting it into temp variable would improve readability (there and in second chunk)

Hello Alexander,

Do you plan to address @kib suggestion and commit it?

Thanks!