Hi Michael,
> On further review of the first patch, I think that it could be a good
> idea to apply the same safeguards within float8in_internal_opt_error.
> Jeevan, what do you think?
>
Sure, agree, it makes sense to address float8in_internal_opt_error(),
there might be more occurrences of such instances in other functions
as well. I think if we agree, as and when encounter them while touching
those areas we should fix them.
What is more dangerous with float8in_internal_opt_error() is, it has
the have_error flag, which is never ever set or used in that function.
Further
more risks are - the callers of this function e.g.
executeItemOptUnwrapTarget()
are passing a non-null pointer to it(default set to false) and expect to
throw
an error if it sees some error during float8in_internal_opt_error(), *but*
float8in_internal_opt_error() has actually never touched the have_error
flag.
So, in this case it is fine because the flag was set to false, if it was not
set, then the garbage value would always result in true and keep on throwing
an error!
Here is relevant code from function executeItemOptUnwrapTarget():
{code}
975 if (jb->type == jbvNumeric)
976 {
977 char *tmp =
DatumGetCString(DirectFunctionCall1(numeric_out,
978
NumericGetDatum(jb->val.numeric)));
979 bool have_error = false;
980
981 (void) float8in_internal_opt_error(tmp,
982 NULL,
983 "double
precision",
984 tmp,
985 &have_error);
986
987 if (have_error)
988 RETURN_ERROR(ereport(ERROR,
989
(errcode(ERRCODE_NON_NUMERIC_JSON_ITEM),
990 errmsg("jsonpath item
method .%s() can only be applied to a numeric value",
991
jspOperationName(jsp->type)))));
992 res = jperOk;
993 }
994 else if (jb->type == jbvString)
995 {
996 /* cast string as double */
997 double val;
998 char *tmp = pnstrdup(jb->val.string.val,
999 jb->val.string.len);
1000 bool have_error = false;
1001
1002 val = float8in_internal_opt_error(tmp,
1003 NULL,
1004 "double
precision",
1005 tmp,
1006 &have_error);
1007
1008 if (have_error || isinf(val))
1009 RETURN_ERROR(ereport(ERROR,
1010
(errcode(ERRCODE_NON_NUMERIC_JSON_ITEM),
1011 errmsg("jsonpath item
method .%s() can only be applied to a numeric value",
1012
jspOperationName(jsp->type)))));
1013
1014 jb = &jbv;
1015 jb->type = jbvNumeric;
1016 jb->val.numeric =
DatumGetNumeric(DirectFunctionCall1(float8_numeric,
1017
Float8GetDatum(val)));
1018 res = jperOk;
1019 }
{code}
I will further check if by mistake any further commits have removed
references
to assignments from float8in_internal_opt_error(), evaluate it, and set out
a
patch.
This is one of the reason, I was saying it can be taken as a good practice
to
let the function who is accepting an out parameter sets the value for sure
to
some or other value.
Regards,
Jeevan Ladhe
