count=65536, rec_size=32768인 악성 pack을 넣어봤어요. -O0에서는 "pack too large"가 나오는데, -O2에서는 "out of memory"가 나와요. 소스는 똑같은데 왜 이런지 도저히 모르겠어요. -O2가 잘못 컴파일하는 건가요, 아니면 malloc이 이상하게 동작하는 건가요?
int total = count * rec_size;
if (total < 0) {
LOG("pack too large");
fclose(f);
return -1;
}
out->data = malloc(total);Hostile pack with count=65536 and rec_size=32768. At -O0 I get "pack too large". At -O2 I get "out of memory" instead. Same source, and I really don't see how. Is -O2 miscompiling it, or is malloc doing something odd?
int total = count * rec_size;
if (total < 0) {
LOG("pack too large");
fclose(f);
return -1;
}
out->data = malloc(total);65536 * 32768은 2^31이라서 signed int에 안 들어가요. 이건 undefined behavior라서 어느 쪽 분기도 "정답"이 아니에요. -O0에서는 우연히 INT_MIN으로 wrap돼서 체크가 걸린 거고요. -O2에서는 컴파일러가 overflow가 없다고 가정하고 < 0 검사를 없애버릴 수 있어요. malloc이 이상한 게 아니에요. 곱하기 전에 count > INT_MAX / rec_size 같은 식으로 검사하거나 size_t를 쓰세요.
65536 * 32768 is 2^31, which doesn't fit in a signed int. That's undefined behavior, so neither branch is "correct". At -O0 it happens to wrap to INT_MIN and your check fires. At -O2 the compiler may assume no overflow and drop the < 0 test. malloc isn't odd. Check before multiplying, e.g. count > INT_MAX / rec_size, or use size_t.
ㅎㅎ 이거 고전이네요. 원인은 e.lund 님이 짚으셨고, 저는 "out of memory" 메시지가 왜 나오는지를 보태볼게요.
컴파일러가 < 0 검사를 없애도 total에는 wrap된 값인 INT_MIN이 그대로 들어 있어요. 그다음 malloc(total)에서 size_t로 변환되는데, 64비트 환경에서는 이게 0xFFFFFFFF80000000이 돼요. 대략 16 엑사바이트쯤이죠. malloc이 거절하니까 OOM 분기가 실행되는 거예요. 그러니까 malloc이 이상한 게 아니라 말도 안 되는 크기를 요청받은 것뿐이에요. (참고로 "NULL만 확인하면 된다"는 쪽이 여기서 안전하다고 착각하게 되는 이유이기도 해요.)
제 질문은 선언 쪽이에요. count와 rec_size의 타입이 뭔가요? 파일에서 uint32_t로 읽은 거라면 곱셈이 unsigned 연산으로 일어나서 합법적으로 wrap돼요. 그러면 int total에 대입하는 건 UB가 아니라 implementation-defined예요. 그렇다면 이야기가 꽤 달라져요.
-O2에서 -fsanitize=undefined는 해보셨어요? 곱셈 지점을 바로 잡아줄 거예요. 그리고 -fwrapv를 주면 두 빌드 결과가 다시 일치하나요?
Ha, this is a classic. e.lund has the cause. I'll add the part that explains your "out of memory" message.
If the compiler drops the < 0 test, total still holds the wrapped value, INT_MIN. Then malloc(total) converts it to size_t. On a 64-bit box that becomes 0xFFFFFFFF80000000, which is about 16 exabytes. malloc says no, and your OOM branch fires. So malloc isn't odd. It's just being asked for something absurd. (Side note: that's also why the "just check for NULL" crowd gets a false sense of safety here.)
My question is about the declarations. What types are count and rec_size? If they're read from the file as uint32_t, the multiply happens in unsigned arithmetic and wraps legally. Then the int total assignment is implementation-defined, not UB. That would change the story a lot.
Have you tried -fsanitize=undefined at -O2? It should flag the multiply directly. And does -fwrapv make the two builds agree again?
< 0 검사는 overflow가 이미 일어난 뒤에 실행되니까, 저라면 그걸 살리려고 하지 않을 거예요. 헤더 값을 곱하기 전에 검증하고, 결과는 size_t에 담으세요.
size_t total;
if (rec_size == 0 ||
__builtin_mul_overflow((size_t)count, (size_t)rec_size, &total) ||
total > max_pack_bytes) {
LOG("pack too large");
fclose(f);
return -1;
}size_t에서는 2^31도 완전히 유효한 크기라서, 악성 pack이 이제는 malloc에 2 GiB를 요청하게 돼요. overflow 검사만으로는 이걸 못 잡아요. max_pack_bytes는 파일 크기나 로더에서 허용하는 예산으로 제한해서, 헤더가 파일에 들어 있는 양보다 많이 주장하지 못하게 하는 게 좋아요.
builtin 문서는 여기 있어요: https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html. GCC와 clang에서 쓸 수 있어요. MSVC로 빌드한다면 대신 count > SIZE_MAX / rec_size 형태를 쓰세요.
The < 0 test runs after the overflow has already happened, so I wouldn't try to rescue it. Validate the header values before you multiply, and keep the result in size_t.
size_t total;
if (rec_size == 0 ||
__builtin_mul_overflow((size_t)count, (size_t)rec_size, &total) ||
total > max_pack_bytes) {
LOG("pack too large");
fclose(f);
return -1;
}With size_t, 2^31 is a perfectly valid size, so your hostile pack would now ask malloc for 2 GiB. Overflow checking alone doesn't catch that. I'd cap max_pack_bytes at the file size, or whatever your loader budget is, so a header can't claim more than the file holds.
The builtin is documented here: https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html. GCC and clang have it. If you build with MSVC, use the count > SIZE_MAX / rec_size form instead.
miscompile 아니에요. signed overflow는 UB라서 컴파일러가 < 0 검사를 지워도 돼요. gcc -O2 -S로 asm을 보면 compare가 아예 없을 거예요.
남는 건 32비트 imul 하나예요. 결과는 0x80000000이고요. 그다음 movslq가 malloc 인자로 넘기려고 sign-extend해서 0xFFFFFFFF80000000이 돼요. malloc이 이걸 못 줘서 OOM 분기로 가는 거예요.
-O0에서는 compare가 그대로 나가서 sign bit를 보니까 "pack too large"가 나오는 거고요.
체크만 고친다고 끝이 아니에요. size_t를 써도 2^31은 256 MB밖에 안 돼서 malloc이 그냥 내줄 거예요. 악성 헤더는 wrap 지점보다 작은 값을 요청하면 그만이에요. 할당하기 전에 실제 파일 길이와 비교하세요. 나중에는 count에도 상한을 두고요.
Not a miscompile. Signed overflow is UB, so the compiler may delete the < 0 test. Look at the asm with gcc -O2 -S and you will see no compare at all.
What is left is a 32-bit imul. The result is 0x80000000. Then movslq sign-extends it for the malloc argument, which gives 0xFFFFFFFF80000000. malloc fails on that, so you land in the OOM branch.
At -O0 the compare is still emitted and sees the sign bit, so you get "pack too large".
Fixing the check is not enough. Even with size_t, 2^31 is only 256 MB, so malloc will happily give it to you. A hostile header can just ask for less than the wrap point. Compare against the real file length before you allocate. Eventually also cap count.
movslq 잘 짚으셨어요. 제가 앞에서 말한 16 엑사바이트 요청으로 INT_MIN이 바뀌는 바로 그 단계예요.
하나만 짚을게요. 2^31 바이트는 256 MB가 아니라 2 GiB예요. drawer6 님 말씀이 맞았어요. 그래서 오히려 님 요점이 더 강해져요. 기본 overcommit인 Linux에서는 2 GiB malloc이 보통 바로 성공해요. 페이지를 아직 건드리지 않았으니까요. 그래서 size_t로 "고친" 버전은 깔끔하게 실패하지 않고 포인터를 돌려줘요. 문제는 나중에 read나 memset이 페이지를 실제로 올릴 때 터지고요. (어떻게 아냐고요? 어떤 클라이언트의 업로드 핸들러가 딱 이랬고, OOM killer가 엉뚱한 프로세스를 죽였어요.)
그러니까 맞아요, 먼저 실제 파일 길이와 비교하세요. fstat으로 크기를 알 수 있어요. 이미 읽은 헤더 바이트를 빼고, 남은 양과 count * rec_size를 overflow-safe한 형태로 비교하면 돼요.
그리고 32비트로 빌드하는 경우 size_t가 32비트라서 unsigned 연산에서 곱셈이 합법적으로 wrap될 수 있어요. 거기서도 overflow 체크는 여전히 중요해요. wrap된 곱이 우연히 작으면 파일 길이 체크만으로는 못 막아요.
Good catch on the movslq. That's the exact step where INT_MIN turns into the 16-exabyte request I was waving at earlier.
One nit, though. 2^31 bytes is 2 GiB, not 256 MB. drawer6 had it right. That makes your point stronger. On Linux with default overcommit, a 2 GiB malloc will usually succeed instantly, because the pages aren't touched yet. So the "fixed" size_t version doesn't fail cleanly. It returns a pointer, and the trouble starts later, when the reads or a memset fault the pages in. (Ask me how I know. A client's upload handler once did exactly this, and the OOM killer took out the wrong process.)
So yes, compare against the real file length first. fstat gives you the size. Subtract the header bytes you've already consumed, and check count * rec_size against what's left, using the overflow-safe form.
Also, if anyone builds this for 32-bit, size_t is 32 bits and the multiply can wrap legally in unsigned arithmetic. The overflow check still matters there. The file-length check alone won't save you if the wrapped product happens to be small.
int total = count * rec_size;
의문이 있는데, 바이트 크기가 왜 애초에 int인가요? 크기는 헤더를 읽는 순간부터 size_t여야 해요. 그리고 count와 rec_size는 파일에서 바로 오는 값이잖아요? 그러면 곱한 뒤에 체크하는 건 설계부터 늦은 거예요. 먼저 -O2에서 UBSan을 돌려 보세요.
int total = count * rec_size;
I have a doubt: why is a byte size an int at all? Sizes should be size_t from the moment you read the header. And count and rec_size come straight from the file, no? Then checking after the multiply is late by design. Run UBSan at -O2 first.
race detector가 틀렸거나, 제가 ack가 보장해 주는 걸 잘못 이해하고 있는 걸까요? pushgate라는 작은 서비스인데, 제 모바일 앱의 푸시 알림용 디바이스 토큰에 붙은 HMAC 서명을 검사해요. 호출하는 쪽이…
저장해 둔 webhook 전송분을 다시 재생해서, 하위 단계에서 신뢰하기 전에 HMAC-SHA256을 검증하는 작은 Node 체커를 만들었어요 (순수 JS, 빌드 단계 없음). 세 개 중 두 개는 통과해요. billing-p…
C로 작은 credential holder를 만들고 있어요 (WSL, Neovim, 별거 없어요). vault_release()는 secret을 지운 다음 free하게 되어 있어요: memset이 분명히 있고 길이도 전체예요…
