Re: Re: PROPOSAL: make PostgreSQL sanitizers-friendly (and prevent information disclosure)
От | Aleksander Alekseev |
---|---|
Тема | Re: Re: PROPOSAL: make PostgreSQL sanitizers-friendly (and prevent information disclosure) |
Дата | |
Msg-id | 20160819125030.42787548@e733 обсуждение исходный текст |
Ответ на | Re: PROPOSAL: make PostgreSQL sanitizers-friendly (and prevent information disclosure) (Heikki Linnakangas <hlinnaka@iki.fi>) |
Ответы |
Re: Re: PROPOSAL: make PostgreSQL sanitizers-friendly (and prevent information disclosure)
|
Список | pgsql-hackers |
Heikki, Peter, thanks a lot for code review! > What's going on here? Surely pg_atomic_init_u64() should initialize > the value? It's because of how pg_atomic_exchange_u64_impl is implemented: ``` while (true) { old = ptr->value; /* <-- reading of uninitialized value! */ if (pg_atomic_compare_exchange_u64_impl(ptr, &old, xchg_)) break; } ``` Currently pg_atomic_init_u64 works like this: pg_atomic_init_u64 `- pg_atomic_init_u64_impl `- pg_atomic_write_u64_impl `- pg_atomic_exchange_u64_impl I suspect there is actually no need to make an atomic exchange during initialization of an atomic variable. Regular `mov` should be enough (IIRC there is no need to do `lock mov` since `mov` is already atomic). Anyway I don't feel brave enough right now to mess with atomic operations since it involves all sort of portability issues. So I removed this change for now. > Why does MemorySanitizer complain about that? Calling stat(2) is > supposed to fill in all the fields we look at, right? > In addition to what Heikki wrote, I think the above is not necessary. It's been my observation that MemorySanitizer often complains on passing structures with uninitialized padding bytes to system calls. I'm not sure whether libc really reads/copies structures in these cases or MemorySanitizer doesn't like the idea in general. Although it's true that maybe MemorySanitizer it too pedantic in these cases, in some respect it's right. Since operating system is a black box from our perspective (not mentioning that there are _many_ OS'es that change all the time) in general case passing a structure with uninitialized padding bytes to a system call can in theory cause a non-repeatable behavior. For instance if there are caching and hash calculation involved. Also, as Chapman noted previously [1], according to PostgreSQL documentation using structures with uninitialized padding bytes should be avoided anyway. I strongly believe that benefits of passing all MemorySanitizer checks (possibility of discovering many complicated bugs automatically in this case) many times outweighs drawbacks of tiny memset's overhead in these concrete cases. > I think this goes too far. You're zeroing all palloc'd memory, even > if it's going to be passed to palloc0(), and zeroed there. It might > even silence legitimate warnings, if there's code somewhere that > does palloc(), and accesses some of it before initializing. Plus > it's a performance hit. Completely agree, my bad. I removed these changes. > Right after this, we have: > VALGRIND_MAKE_MEM_DEFINED(&msg, sizeof(msg)); > Do we need both? Apparently we don't. I removed VALGRIND_MAKE_MEM_DEFINED here since now there are no uninitialized padding bytes in &msg. Corrected patch is attached. If you have any other ideas how it could be improved please let me know! [1] https://www.postgresql.org/message-id/56EFF347.20500%40anastigmatix.net -- Best regards, Aleksander Alekseev
Вложения
В списке pgsql-hackers по дате отправления: