Skip to content

Improved error checking for function return values #1355

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lsh123 merged 1 commit into from
Oct 1, 2026
Merged

Improved error checking for function return values #1355

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 59 additions & 40 deletions apps/xmlsec.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include <stdlib.h>
#include <string.h>
#include <time.h>
#include <limits.h>

#if defined(_MSC_VER) && _MSC_VER < 1900
#define snprintf _snprintf
Expand All @@ -18,6 +19,7 @@
#include <libxml/parser.h>
#include <libxml/xpath.h>
#include <libxml/xpathInternals.h>
#include <libxml/xmlIO.h>

#ifndef XMLSEC_NO_XSLT
#include <libxslt/xslt.h>
Expand Down Expand Up @@ -1316,16 +1318,9 @@ xmlSecAppSignFile(const char* filename) {
total_time += clock() - start_time;

if(repeats <= 1) {
FILE* f;

f = xmlSecAppOpenFile(xmlSecAppCmdLineParamGetString(&outputParam));
if(f == NULL) {
fprintf(stderr,"Error: failed to open output file \"%s\"\n",
xmlSecAppCmdLineParamGetString(&outputParam));
if(xmlSecAppWriteResult(data->doc, NULL) < 0) {
goto done;
}
xmlDocDump(f, data->doc);
xmlSecAppCloseFile(f);
}

res = 0;
Expand Down Expand Up @@ -1368,37 +1363,22 @@ xmlSecAppVerifyFile(const char* filename) {
goto done;
}

/* sign */
/* verify */
start_time = clock();
if(xmlSecDSigCtxVerify(&dsigCtx, data->startNode) < 0) {
fprintf(stderr,"Error: signature failed \n");
goto done;
}
total_time += clock() - start_time;

if((repeats <= 1) && (dsigCtx.status != xmlSecDSigStatusSucceeded)){
if((repeats <= 1) && (dsigCtx.status != xmlSecDSigStatusSucceeded)) {
/* return an error if signature does not match */
goto done;
}

if(repeats <= 1) {
FILE* f;

/*
* Note: the output file must be opened before the "done:" label.
* Otherwise a failed open would "goto done" and re-enter this block,
* retrying the same failing open forever.
*/
f = xmlSecAppOpenFile(xmlSecAppCmdLineParamGetString(&outputParam));
if(f == NULL) {
fprintf(stderr,"Error: failed to open output file \"%s\"\n",
xmlSecAppCmdLineParamGetString(&outputParam));
goto done;
}
xmlSecAppCloseFile(f);
}

/* success */
res = 0;

done:
/* print debug info if requested */
if(repeats <= 1) {
Expand Down Expand Up @@ -1549,16 +1529,9 @@ xmlSecAppSignTmpl(void) {
total_time += clock() - start_time;

if(repeats <= 1) {
FILE* f;

f = xmlSecAppOpenFile(xmlSecAppCmdLineParamGetString(&outputParam));
if(f == NULL) {
fprintf(stderr,"Error: failed to open output file \"%s\"\n",
xmlSecAppCmdLineParamGetString(&outputParam));
if(xmlSecAppWriteResult(doc, NULL) < 0) {
goto done;
}
xmlDocDump(f, doc);
xmlSecAppCloseFile(f);
}

res = 0;
Expand Down Expand Up @@ -3073,21 +3046,67 @@ xmlSecAppCloseFile(FILE* file) {
static int
xmlSecAppWriteResult(xmlDocPtr doc, xmlSecBufferPtr buffer) {
FILE* f;
xmlOutputBufferPtr outBuffer;
int ret;

f = xmlSecAppOpenFile(xmlSecAppCmdLineParamGetString(&outputParam));
if(f == NULL) {
return(-1);
}

outBuffer = xmlOutputBufferCreateFile(f, NULL);
if(outBuffer == NULL) {
fprintf(stderr, "Error: failed to create output buffer\n");
xmlSecAppCloseFile(f);
return(-1);
}

/* dump output */
if(doc != NULL) {
xmlDocDump(f, doc);
} else if((buffer != NULL) && (xmlSecBufferGetData(buffer) != NULL)) {
(void)fwrite(xmlSecBufferGetData(buffer), xmlSecBufferGetSize(buffer), 1, f);
ret = xmlSaveFileTo(outBuffer, doc, (const char*)doc->encoding);
if (ret < 0) {
fprintf(stderr, "Error: failed to write xml output\n");
/* xmlSaveFileTo closes the buffer and the file */
return(-1);
}
/* xmlSaveFileTo closes the buffer and the file */
} else if(buffer != NULL) {
xmlSecSize bufSize;
const xmlSecByte* bufData;

bufData = xmlSecBufferGetData(buffer);
bufSize = xmlSecBufferGetSize(buffer);
if((bufData == NULL) && (bufSize != 0)) {
fprintf(stderr, "Error: buffer data is NULL but buffer size is not zero\n");
/* xmlOutputBufferClose closes the file */
(void)xmlOutputBufferClose(outBuffer);
return(-1);
}
if(bufSize > (size_t)INT_MAX) {
fprintf(stderr, "Error: binary output size exceeds int limit\n");
/* xmlOutputBufferClose closes the file */
(void)xmlOutputBufferClose(outBuffer);
return(-1);
}
if(bufData != NULL) {
ret = xmlOutputBufferWrite(outBuffer, (int)bufSize, (const char*)bufData);
if (ret < 0) {
/* xmlOutputBufferClose closes the file */
fprintf(stderr, "Error: failed to write binary output\n");
(void)xmlOutputBufferClose(outBuffer);
return(-1);
}
}
/* xmlOutputBufferClose closes the file */
(void)xmlOutputBufferClose(outBuffer);
} else {
fprintf(stderr, "Error: both result doc and result buffer are null\n");
xmlSecAppCloseFile(f);
/* xmlOutputBufferClose closes the file */
(void)xmlOutputBufferClose(outBuffer);
return(-1);
}
xmlSecAppCloseFile(f);

/* done */
return(0);
}

Expand Down
7 changes: 7 additions & 0 deletions src/bn.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -825,6 +825,9 @@ xmlSecBnGetNodeValue(xmlSecBnPtr bn, xmlNodePtr cur, xmlSecBnFormat format, int
}
xmlFree(content);
break;
default:
xmlSecInvalidDataError("unsupported BN format", NULL);
return(-1);
}

if(reverse != 0) {
Expand Down Expand Up @@ -897,6 +900,10 @@ xmlSecBnSetNodeValue(xmlSecBnPtr bn, xmlNodePtr cur, xmlSecBnFormat format, int
xmlNodeSetContent(cur, content);
xmlFree(content);
break;
default:
/* invalid format */
xmlSecInvalidDataError("unsupported BN format", NULL);
return(-1);
}

if(addLineBreaks) {
Expand Down
7 changes: 3 additions & 4 deletions src/c14n.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,11 +278,10 @@ xmlSecTransformC14NPopBin(xmlSecTransformPtr transform, xmlSecByte* data,

xmlSecAssert2(transform->inNodes == NULL, -1);

/* todo: isn't it an error? */
/* a C14N transform with no previous transform has no input to canonicalize */
if(transform->prev == NULL) {
(*dataSize) = 0;
transform->status = xmlSecTransformStatusFinished;
return(0);
xmlSecInternalError("xmlSecTransformC14NPopBin", xmlSecTransformGetName(transform));
return(-1);
}

/* get xml data from previous transform */
Expand Down
6 changes: 6 additions & 0 deletions src/dl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,6 +391,12 @@ int
xmlSecCryptoDLShutdown(void) {
int ret;

if(!xmlSecPtrListIsValid(&gXmlSecCryptoDLLibraries)) {
/* the dynamic loading engine was not initialized */
gXmlSecCryptoDLFunctions = NULL;
return(0);
}

xmlSecPtrListFinalize(&gXmlSecCryptoDLLibraries);
gXmlSecCryptoDLFunctions = NULL;

Expand Down
6 changes: 5 additions & 1 deletion src/gnutls/asymkeys.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,11 @@ xmlSecGnuTLSKeyDataRsaAdoptPrivateKey(xmlSecKeyDataPtr data, gnutls_x509_privkey
/* First check that p < q; if not swap p and q and recompute u. */
if (gcry_mpi_cmp(mpis[3], mpis[4]) > 0) {
gcry_mpi_swap(mpis[3], mpis[4]);
gcry_mpi_invm(mpis[5], mpis[3], mpis[4]);
if(!gcry_mpi_invm(mpis[5], mpis[3], mpis[4])) {
xmlSecGnuTLSGCryptError("gcry_mpi_invm", (gcry_error_t)GPG_ERR_NO_ERROR, NULL);
xmlSecGnuTLSDestroyMpis(mpis, sizeof(mpis)/sizeof(mpis[0]));
return(-1);
}
}

/* build expressions */
Expand Down
10 changes: 9 additions & 1 deletion src/io.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -603,6 +603,7 @@ xmlSecTransformInputURIOpen(xmlSecTransformPtr transform, const xmlChar *uri) {
int
xmlSecTransformInputURIClose(xmlSecTransformPtr transform) {
xmlSecInputURICtxPtr ctx;
int ret;

xmlSecAssert2(xmlSecTransformCheckId(transform, xmlSecTransformInputURIId), -1);

Expand All @@ -611,7 +612,14 @@ xmlSecTransformInputURIClose(xmlSecTransformPtr transform) {

/* close if still open and mark as closed */
if((ctx->clbksCtx != NULL) && (ctx->clbks != NULL) && (ctx->clbks->closecallback != NULL)) {
(ctx->clbks->closecallback)(ctx->clbksCtx);
ret = (ctx->clbks->closecallback)(ctx->clbksCtx);
if(ret != 0) {
xmlSecIOError("ctx->clbks->closecallback", xmlSecTransformGetName(transform), NULL);
/* mark as closed to prevent a second close attempt on finalize */
ctx->clbksCtx = NULL;
ctx->clbks = NULL;
return(-1);
}
}
ctx->clbksCtx = NULL;
ctx->clbks = NULL;
Expand Down
44 changes: 24 additions & 20 deletions src/keyinfo.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -639,6 +639,7 @@ xmlSecKeyDataNameGetKlass(void) {
static int
xmlSecKeyDataNameXmlRead(xmlSecKeyDataId id, xmlSecKeyPtr key, xmlNodePtr node, xmlSecKeyInfoCtxPtr keyInfoCtx) {
xmlChar* newName;
const xmlChar* oldName;
int ret;

xmlSecAssert2(id == xmlSecKeyDataNameId, -1);
Expand Down Expand Up @@ -687,29 +688,32 @@ xmlSecKeyDataNameXmlRead(xmlSecKeyDataId id, xmlSecKeyPtr key, xmlNodePtr node,
xmlFree(newName);
return(-1);
}

/* done */
xmlFree(newName);
return(0);
}
/* key not found in the manager; fall through to set the name anyway */
/* TODO: record the key names we tried */
} else {
const xmlChar* oldName;
}

/* if we already have a keyname, make sure that it matches or set it */
oldName = xmlSecKeyGetName(key);
if(oldName != NULL) {
if(!xmlStrEqual(oldName, newName)) {
xmlSecOtherError(XMLSEC_ERRORS_R_INVALID_KEY_DATA,
xmlSecKeyDataKlassGetName(id),
"key name is already specified");
xmlFree(newName);
return(-1);
}
} else {
ret = xmlSecKeySetName(key, newName);
if(ret < 0) {
xmlSecInternalError("xmlSecKeySetName",
xmlSecKeyDataKlassGetName(id));
xmlFree(newName);
return(-1);
}
/* if we already have a keyname, make sure that it matches or set it */
oldName = xmlSecKeyGetName(key);
if(oldName != NULL) {
if(!xmlStrEqual(oldName, newName)) {
xmlSecOtherError(XMLSEC_ERRORS_R_INVALID_KEY_DATA,
xmlSecKeyDataKlassGetName(id),
"key name is already specified");
xmlFree(newName);
return(-1);
}
} else {
ret = xmlSecKeySetName(key, newName);
if(ret < 0) {
xmlSecInternalError("xmlSecKeySetName",
xmlSecKeyDataKlassGetName(id));
xmlFree(newName);
return(-1);
}
}

Expand Down
12 changes: 8 additions & 4 deletions src/mscng/certkeys.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -893,8 +893,8 @@ xmlSecMSCngKeyDataDsaGenerate(xmlSecKeyDataPtr data, xmlSecSize sizeBits,
int ret;
int res = -1;

xmlSecAssert2(xmlSecKeyDataIsValid(data), xmlSecKeyDataTypeUnknown);
xmlSecAssert2(xmlSecKeyDataCheckSize(data, xmlSecMSCngKeyDataSize), xmlSecKeyDataTypeUnknown);
xmlSecAssert2(xmlSecKeyDataIsValid(data), -1);
xmlSecAssert2(xmlSecKeyDataCheckSize(data, xmlSecMSCngKeyDataSize), -1);
xmlSecAssert2(xmlSecKeyDataCheckId(data, xmlSecMSCngKeyDataDsaId), -1);
xmlSecAssert2(sizeBits > 0, -1);

Expand Down Expand Up @@ -1296,6 +1296,10 @@ xmlSecMSCngKeyDataGetSize(xmlSecKeyDataPtr data) {
xmlSecAssert2(ctx->cert->pCertInfo != NULL, 0);
length = CertGetPublicKeyLength(X509_ASN_ENCODING | PKCS_7_ASN_ENCODING,
&ctx->cert->pCertInfo->SubjectPublicKeyInfo);
if(length == 0) {
xmlSecMSCngLastError("CertGetPublicKeyLength", NULL);
return(0);
}
} else if(ctx->pubkey != 0) {
DWORD lenlen = sizeof(length);
status = BCryptGetProperty(ctx->pubkey,
Expand Down Expand Up @@ -1372,8 +1376,8 @@ xmlSecMSCngKeyDataRsaGenerate(xmlSecKeyDataPtr data, xmlSecSize sizeBits,
int ret;
int res = -1;

xmlSecAssert2(xmlSecKeyDataIsValid(data), xmlSecKeyDataTypeUnknown);
xmlSecAssert2(xmlSecKeyDataCheckSize(data, xmlSecMSCngKeyDataSize), xmlSecKeyDataTypeUnknown);
xmlSecAssert2(xmlSecKeyDataIsValid(data), -1);
xmlSecAssert2(xmlSecKeyDataCheckSize(data, xmlSecMSCngKeyDataSize), -1);
xmlSecAssert2(xmlSecKeyDataCheckId(data, xmlSecMSCngKeyDataRsaId), -1);
xmlSecAssert2(sizeBits > 0, -1);

Expand Down
20 changes: 16 additions & 4 deletions src/mscng/keysstore.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,10 @@ xmlSecMSCngKeysStoreFindCert(xmlSecKeyStorePtr store, const xmlChar* name,
if(wcName == NULL) {
xmlSecInternalError("xmlSecWin32ConvertUtf8ToTstr(name)",
xmlSecKeyStoreGetName(store));
CertCloseStore(hStore, 0);
if(!CertCloseStore(hStore, 0)) {
xmlSecMSCngLastError("CertCloseStore",
xmlSecKeyStoreGetName(store));
}
return(NULL);
}

Expand All @@ -137,7 +140,10 @@ xmlSecMSCngKeysStoreFindCert(xmlSecKeyStorePtr store, const xmlChar* name,
xmlSecInternalError("xmlSecWin32ConvertUtf8ToUnicode(name)",
xmlSecKeyStoreGetName(store));
xmlFree(wcName);
CertCloseStore(hStore, 0);
if(!CertCloseStore(hStore, 0)) {
xmlSecMSCngLastError("CertCloseStore",
xmlSecKeyStoreGetName(store));
}
return(NULL);
}

Expand All @@ -159,7 +165,10 @@ xmlSecMSCngKeysStoreFindCert(xmlSecKeyStorePtr store, const xmlChar* name,
xmlSecMallocError(dwPropSize, xmlSecKeyStoreGetName(store));
xmlFree(lpwName);
xmlFree(wcName);
CertCloseStore(hStore, 0);
if(!CertCloseStore(hStore, 0)) {
xmlSecMSCngLastError("CertCloseStore",
xmlSecKeyStoreGetName(store));
}
CertFreeCertificateContext(pCertCtxIter);
return(NULL);
}
Expand Down Expand Up @@ -200,7 +209,10 @@ xmlSecMSCngKeysStoreFindCert(xmlSecKeyStorePtr store, const xmlChar* name,
xmlFree(wcName);
/* dwFlags=0 means close the store with memory remaining allocated for
* contexts that have not been freed */
CertCloseStore(hStore, 0);
if(!CertCloseStore(hStore, 0)) {
xmlSecMSCngLastError("CertCloseStore",
xmlSecKeyStoreGetName(store));
}

return(pCertContext);
#else /* XMLSEC_NO_X509 */
Expand Down
Loading
Loading