8000 Fix allocation logic of cryptohash context data with OpenSSL · postgres/postgres@55fe26a · GitHub
[go: up one dir, main page]
Skip to content
8000

Commit 55fe26a

Browse files
michaelpqmichaelpq
committed
Fix allocation logic of cryptohash context data with OpenSSL
The allocation of the cryptohash context data when building with OpenSSL was happening in the memory context of the caller of pg_cryptohash_create(), which could lead to issues with resowner cleanup if cascading resources are cleaned up on an error. Like other facilities using resowners, move the base allocation to TopMemoryContext to ensure a correct cleanup on failure. The resulting code gets simpler with this commit as the context data is now hold by a unique opaque pointer, so as there is only one single allocation done in TopMemoryContext. After discussion, also change the cryptohash subroutines to return an error if the caller provides NULL for the context data to ease error detection on OOM. Author: Heikki Linnakangas Discussion: https://postgr.es/m/X9xbuEoiU3dlImfa@paquier.xyz
1 parent 9877374 commit 55fe26a

File tree

4 files changed

+81
-125
lines changed

4 files changed

+81
-125
lines changed

‎src/common/cryptohash.c

Lines changed: 44 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,21 @@
3939
#define FREE(ptr) free(ptr)
4040
#endif
4141

42+
/* Internal pg_cryptohash_ctx structure */
43+
struct pg_cryptohash_ctx
44+
{
45+
pg_cryptohash_type type;
46+
47+
union
48+
{
49+
pg_md5_ctx md5;
50+
pg_sha224_ctx sha224;
51+
pg_sha256_ctx sha256;
52+
pg_sha384_ctx sha384;
53+
pg_sha512_ctx sha512;
54+
} data;
55+
};
56+
4257
/*
4358
* pg_cryptohash_create
4459
*
@@ -50,38 +65,18 @@ pg_cryptohash_create(pg_cryptohash_type type)
5065
{
5166
pg_cryptohash_ctx *ctx;
5267

68+
/*
69+
* Note that this always allocates enough space for the largest hash. A
70+
* smaller allocation would be enough for md5, sha224 and sha256, but the
71+
* small extra amount of memory does not make it worth complicating this
72+
* code.
73+
*/
5374
ctx = ALLOC(sizeof(pg_cryptohash_ctx));
5475
if (ctx == NULL)
5576
return NULL;
56-
77+
memset(ctx, 0, sizeof(pg_cryptohash_ctx));
5778
ctx->type = type;
5879

59-
switch (type)
60-
{
61-
case PG_MD5:
62-
ctx->data = ALLOC(sizeof(pg_md5_ctx));
63-
break;
64-
case PG_SHA224:
65-
ctx->data = ALLOC(sizeof(pg_sha224_ctx));
66-
break;
67-
case PG_SHA256:
68-
ctx->data = ALLOC(sizeof(pg_sha256_ctx));
69-
break;
70-
case PG_SHA384:
71-
ctx->data = ALLOC(sizeof(pg_sha384_ctx));
72-
break;
73-
case PG_SHA512:
74-
ctx->data = ALLOC(sizeof(pg_sha512_ctx));
75-
break;
76-
}
77-
78-
if (ctx->data == NULL)
79-
{
80-
explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
81-
FREE(ctx);
82-
return NULL;
83-
}
84-
8580
return ctx;
8681
}
8782

@@ -95,24 +90,24 @@ int
9590
pg_cryptohash_init(pg_cryptohash_ctx *ctx)
9691
{
9792
if (ctx == NULL)
98-
return 0;
93+
return -1;
9994

10095
switch (ctx->type)
10196
{
10297
case PG_MD5:
103-
pg_md5_init((pg_md5_ctx *) ctx->data);
98+
pg_md5_init(&ctx->data.md5);
10499
break;
105100
case PG_SHA224:
106-
pg_sha224_init((pg_sha224_ctx *) ctx->data);
101+
pg_sha224_init(&ctx->data.sha224);
107102
break;
108103
case PG_SHA256:
109-
pg_sha256_init((pg_sha256_ctx *) ctx->data);
104+
pg_sha256_init(&ctx->data.sha256);
110105
break;
111106
case PG_SHA384:
112-
pg_sha384_init((pg_sha384_ctx *) ctx->data);
107+
pg_sha384_init(&ctx->data.sha384);
113108
break;
114109
case PG_SHA512:
115-
pg_sha512_init((pg_sha512_ctx *) ctx->data);
110+
pg_sha512_init(&ctx->data.sha512);
116111
break;
117112
}
118113

@@ -123,30 +118,31 @@ pg_cryptohash_init(pg_cryptohash_ctx *ctx)
123118
* pg_cryptohash_update
124119
*
125120
* Update a hash context. Note that this implementation is designed
126-
* to never fail, so this always returns 0.
121+
* to never fail, so this always returns 0 except if the caller has
122+
* given a NULL context.
127123
*/
128124
int
129125
pg_cryptohash_update(pg_cryptohash_ctx *ctx, const uint8 *data, size_t len)
130126
{
131127
if (ctx == NULL)
132-
return 0;
128+
return -1;
133129

134130
switch (ctx->type)
135131
{
136132
case PG_MD5:
137-
pg_md5_update((pg_md5_ctx *) ctx->data, data, len);
133+
pg_md5_update(&ctx->data.md5, data, len);
138134
break;
139135
case PG_SHA224:
140-
pg_sha224_update((pg_sha224_ctx *) ctx->data, data, len);
136+
pg_sha224_update(&ctx->data.sha224, data, len);
141137
break;
142138
case PG_SHA256:
143-
pg_sha256_update((pg_sha256_ctx *) ctx->data, data, len);
139+
pg_sha256_update(&ctx->data.sha256, data, len);
144140
break;
145141
case PG_SHA384:
146-
pg_sha384_update((pg_sha384_ctx *) ctx->data, data, len);
142+
pg_sha384_update(&ctx->data.sha384, data, len);
147143
break;
148144
case PG_SHA512:
149-
pg_sha512_update((pg_sha512_ctx *) ctx->data, data, len);
145+
pg_sha512_update(&ctx->data.sha512, data, len);
150146
break;
151147
}
152148

@@ -157,30 +153,31 @@ pg_cryptohash_update(pg_cryptohash_ctx *ctx, const uint8 *data, size_t len)
157153
* pg_cryptohash_final
158154
*
159155
* Finalize a hash context. Note that this implementation is designed
160-
* to never fail, so this always returns 0.
156+
* to never fail, so this always returns 0 except if the caller has
157+
* given a NULL context.
161158
*/
162159
int
163160
pg_cryptohash_final(pg_cryptohash_ctx *ctx, uint8 *dest)
164161
{
165162
if (ctx == NULL)
166-
return 0;
163+
return -1;
167164

168165
switch (ctx->type)
169166
{
170167
case PG_MD5:
171-
pg_md5_final((pg_md5_ctx *) ctx->data, dest);
168+
pg_md5_final(&ctx->data.md5, dest);
172169
break;
173170
case PG_SHA224:
174-
pg_sha224_final((pg_sha224_ctx *) ctx->data, dest);
171+
pg_sha224_final(&ctx->data.sha224, dest);
175172
break;
176173
case PG_SHA256:
177-
pg_sha256_final((pg_sha256_ctx *) ctx->data, dest);
174+
pg_sha256_final(&ctx->data.sha256, dest);
178175
break;
179176
case PG_SHA384:
180-
pg_sha384_final((pg_sha384_ctx *) ctx->data, dest);
177+
pg_sha384_final(&ctx->data.sha384, dest);
181178
break;
182179
case PG_SHA512:
183-
pg_sha512_final((pg_sha512_ctx *) ctx->data, dest);
180+
pg_sha512_final(&ctx->data.sha512, dest);
184181
break;
185182
}
186183

@@ -198,26 +195,6 @@ pg_cryptohash_free(pg_cryptohash_ctx *ctx)
198195
if (ctx == NULL)
199196
return;
200197

201-
switch (ctx->type)
202-
{
203-
case PG_MD5:
204-
explicit_bzero(ctx->data, sizeof(pg_md5_ctx));
205-
break;
206-
case PG_SHA224:
207-
explicit_bzero(ctx->data, sizeof(pg_sha224_ctx));
208-
break;
209-
case PG_SHA256:
210-
explicit_bzero(ctx->data, sizeof(pg_sha256_ctx));
211-
break;
212-
case PG_SHA384:
213-
explicit_bzero(ctx->data, sizeof(pg_sha384_ctx));
214-
break;
215-
case PG_SHA512:
216-
explicit_bzero(ctx->data, sizeof(pg_sha512_ctx));
217-
break;
218-
}
219-
220-
FREE(ctx->data);
221198
explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
222199
FREE(ctx);
223200
}

0 commit comments

Comments
 (0)
0