[xmlsec] How to avoid the node passed to xmlSecEncCtxXmlEncrypt
to be released
Aleksey Sanin
aleksey at aleksey.com
Wed Jun 4 08:05:34 PDT 2008
Sorry for delay with reviewing the patch! As I wrote before
it completely disappeared from my radar for some reasons.
I've applied and commited your patch but I've had to make
some significant changes:
1) I removed "enum xmlEncCtxNodeReplacementMode" and used
the "flags" variable in the encryption context instead. This
provides better backward compatibility because the structure
size does not change.
2) For the same reason, I replaced "reserved0" pointer with
"replacedNodeList".
3) Again, for backward compatibility, I've kept the old versions
of xmlSecXxxReplace() functions and added new
xmlSecXxxReplaceAndReturn() functions that provide the new
functionality.
4) There was a bug in xmlSecReplaceContent() implementation:
when you copied nodes one by one you forgot to move "cur"
variable (i.e. the tail) after you've added the new node.
As the result, if the original nodes list looks like
1,2,3,4 then the returned result is 1,4,3,2.
5) I've also made some minor cosmetic changes (spaces, brackets,
etc.) to match the xmlsec code style.
Just in case, I am attaching the resulting patch. Please let me know
if you find any problems with my changes!
Thank you again for the patch!
Aleksey
-------------- next part --------------
Index: src/xmltree.c
===================================================================
--- src/xmltree.c (revision 988)
+++ src/xmltree.c (working copy)
@@ -419,6 +419,21 @@
*/
int
xmlSecReplaceNode(xmlNodePtr node, xmlNodePtr newNode) {
+ return xmlSecReplaceNodeAndReturn(node, newNode, NULL);
+}
+
+/**
+ * xmlSecReplaceNodeAndReturn:
+ * @node: the current node.
+ * @newNode: the new node.
+ * @replaced: the replaced node, or release it if NULL is given
+ *
+ * Swaps the @node and @newNode in the XML tree.
+ *
+ * Returns 0 on success or a negative value if an error occurs.
+ */
+int
+xmlSecReplaceNodeAndReturn(xmlNodePtr node, xmlNodePtr newNode, xmlNodePtr* replaced) {
xmlNodePtr oldNode;
int restoreRoot = 0;
@@ -448,7 +463,13 @@
xmlDocSetRootElement(oldNode->doc, newNode);
}
- xmlFreeNode(oldNode);
+ /* return the old node if requested */
+ if(replaced != NULL) {
+ (*replaced) = oldNode;
+ } else {
+ xmlFreeNode(oldNode);
+ }
+
return(0);
}
@@ -463,19 +484,55 @@
*/
int
xmlSecReplaceContent(xmlNodePtr node, xmlNodePtr newNode) {
+ return xmlSecReplaceContentAndReturn(node, newNode, NULL);
+}
+
+/**
+ * xmlSecReplaceContentAndReturn
+ * @node: the current node.
+ * @newNode: the new node.
+ * @replaced: the replaced nodes, or release them if NULL is given
+ *
+ * Swaps the content of @node and @newNode.
+ *
+ * Returns 0 on success or a negative value if an error occurs.
+ */
+int
+xmlSecReplaceContentAndReturn(xmlNodePtr node, xmlNodePtr newNode, xmlNodePtr *replaced) {
xmlSecAssert2(node != NULL, -1);
xmlSecAssert2(newNode != NULL, -1);
xmlUnlinkNode(newNode);
xmlSetTreeDoc(newNode, node->doc);
- xmlNodeSetContent(node, NULL);
+
+ /* return the old nodes if requested */
+ if(replaced != NULL) {
+ xmlNodePtr cur, next, tail;
+
+ (*replaced) = tail = NULL;
+ for(cur = node->children; (cur != NULL); cur = next) {
+ next = cur->next;
+ if((*replaced) != NULL) {
+ /* n is unlinked in this function */
+ xmlAddNextSibling(tail, cur);
+ tail = cur;
+ } else {
+ /* this is the first node, (*replaced) is the head */
+ xmlUnlinkNode(cur);
+ (*replaced) = tail = cur;
+ }
+ }
+ } else {
+ /* just delete the content */
+ xmlNodeSetContent(node, NULL);
+ }
+
xmlAddChild(node, newNode);
xmlSetTreeDoc(newNode, node->doc);
return(0);
}
-
/**
* xmlSecReplaceNodeBuffer:
* @node: the current node.
@@ -487,8 +544,23 @@
* Returns 0 on success or a negative value if an error occurs.
*/
int
-xmlSecReplaceNodeBuffer(xmlNodePtr node,
- const xmlSecByte *buffer, xmlSecSize size) {
+xmlSecReplaceNodeBuffer(xmlNodePtr node, const xmlSecByte *buffer, xmlSecSize size) {
+ return xmlSecReplaceNodeBufferAndReturn(node, buffer, size, NULL);
+}
+
+/**
+ * xmlSecReplaceNodeBufferAndReturn:
+ * @node: the current node.
+ * @buffer: the XML data.
+ * @size: the XML data size.
+ * @replaced: the replaced nodes, or release them if NULL is given
+ *
+ * Swaps the @node and the parsed XML data from the @buffer in the XML tree.
+ *
+ * Returns 0 on success or a negative value if an error occurs.
+ */
+int
+xmlSecReplaceNodeBufferAndReturn(xmlNodePtr node, const xmlSecByte *buffer, xmlSecSize size, xmlNodePtr *replaced) {
xmlNodePtr results = NULL;
xmlNodePtr next = NULL;
@@ -514,8 +586,14 @@
/* remove old node */
xmlUnlinkNode(node);
- xmlFreeNode(node);
+ /* return the old node if requested */
+ if(replaced != NULL) {
+ (*replaced) = node;
+ } else {
+ xmlFreeNode(node);
+ }
+
return(0);
}
Index: src/xmlenc.c
===================================================================
--- src/xmlenc.c (revision 988)
+++ src/xmlenc.c (working copy)
@@ -192,34 +192,45 @@
encCtx->resultBase64Encoded = 0;
encCtx->resultReplaced = 0;
encCtx->encMethod = NULL;
+
+ if (encCtx->replacedNodeList != NULL) {
+ xmlFreeNodeList(encCtx->replacedNodeList);
+ encCtx->replacedNodeList = NULL;
+ }
+
if(encCtx->encKey != NULL) {
- xmlSecKeyDestroy(encCtx->encKey);
- encCtx->encKey = NULL;
+ xmlSecKeyDestroy(encCtx->encKey);
+ encCtx->encKey = NULL;
}
if(encCtx->id != NULL) {
- xmlFree(encCtx->id);
- encCtx->id = NULL;
+ xmlFree(encCtx->id);
+ encCtx->id = NULL;
}
+
if(encCtx->type != NULL) {
- xmlFree(encCtx->type);
- encCtx->type = NULL;
+ xmlFree(encCtx->type);
+ encCtx->type = NULL;
}
+
if(encCtx->mimeType != NULL) {
- xmlFree(encCtx->mimeType);
- encCtx->mimeType = NULL;
+ xmlFree(encCtx->mimeType);
+ encCtx->mimeType = NULL;
}
+
if(encCtx->encoding != NULL) {
- xmlFree(encCtx->encoding);
- encCtx->encoding = NULL;
+ xmlFree(encCtx->encoding);
+ encCtx->encoding = NULL;
}
+
if(encCtx->recipient != NULL) {
- xmlFree(encCtx->recipient);
- encCtx->recipient = NULL;
+ xmlFree(encCtx->recipient);
+ encCtx->recipient = NULL;
}
+
if(encCtx->carriedKeyName != NULL) {
- xmlFree(encCtx->carriedKeyName);
- encCtx->carriedKeyName = NULL;
+ xmlFree(encCtx->carriedKeyName);
+ encCtx->carriedKeyName = NULL;
}
encCtx->encDataNode = encCtx->encMethodNode =
@@ -445,43 +456,73 @@
"xmlSecEncCtxEncDataNodeWrite",
XMLSEC_ERRORS_R_XMLSEC_FAILED,
XMLSEC_ERRORS_NO_MESSAGE);
- return(-1);
+ return(-1);
}
/* now we need to update our original document */
if((encCtx->type != NULL) && xmlStrEqual(encCtx->type, xmlSecTypeEncElement)) {
- ret = xmlSecReplaceNode(node, tmpl);
- if(ret < 0) {
- xmlSecError(XMLSEC_ERRORS_HERE,
- NULL,
- "xmlSecReplaceNode",
- XMLSEC_ERRORS_R_XMLSEC_FAILED,
- "node=%s",
- xmlSecErrorsSafeString(xmlSecNodeGetName(node)));
- return(-1);
- }
- encCtx->resultReplaced = 1;
+ /* check if we need to return the replaced node */
+ if((encCtx->flags & XMLSEC_ENC_RETURN_REPLACED_NODE) != 0) {
+ ret = xmlSecReplaceNodeAndReturn(node, tmpl, &(encCtx->replacedNodeList));
+ if(ret < 0) {
+ xmlSecError(XMLSEC_ERRORS_HERE,
+ NULL,
+ "xmlSecReplaceNode",
+ XMLSEC_ERRORS_R_XMLSEC_FAILED,
+ "node=%s",
+ xmlSecErrorsSafeString(xmlSecNodeGetName(node)));
+ return(-1);
+ }
+ } else {
+ ret = xmlSecReplaceNode(node, tmpl);
+ if(ret < 0) {
+ xmlSecError(XMLSEC_ERRORS_HERE,
+ NULL,
+ "xmlSecReplaceNode",
+ XMLSEC_ERRORS_R_XMLSEC_FAILED,
+ "node=%s",
+ xmlSecErrorsSafeString(xmlSecNodeGetName(node)));
+ return(-1);
+ }
+ }
+
+ encCtx->resultReplaced = 1;
} else if((encCtx->type != NULL) && xmlStrEqual(encCtx->type, xmlSecTypeEncContent)) {
- ret = xmlSecReplaceContent(node, tmpl);
- if(ret < 0) {
+ /* check if we need to return the replaced node */
+ if((encCtx->flags & XMLSEC_ENC_RETURN_REPLACED_NODE) != 0) {
+ ret = xmlSecReplaceContentAndReturn(node, tmpl, &(encCtx->replacedNodeList));
+ if(ret < 0) {
+ xmlSecError(XMLSEC_ERRORS_HERE,
+ NULL,
+ "xmlSecReplaceContentAndReturn",
+ XMLSEC_ERRORS_R_XMLSEC_FAILED,
+ "node=%s",
+ xmlSecErrorsSafeString(xmlSecNodeGetName(node)));
+ return(-1);
+ }
+ } else {
+ ret = xmlSecReplaceContent(node, tmpl);
+ if(ret < 0) {
+ xmlSecError(XMLSEC_ERRORS_HERE,
+ NULL,
+ "xmlSecReplaceContent",
+ XMLSEC_ERRORS_R_XMLSEC_FAILED,
+ "node=%s",
+ xmlSecErrorsSafeString(xmlSecNodeGetName(node)));
+ return(-1);
+ }
+ }
+
+ encCtx->resultReplaced = 1;
+ } else {
+ /* we should've catached this error before */
xmlSecError(XMLSEC_ERRORS_HERE,
- NULL,
- "xmlSecReplaceContent",
- XMLSEC_ERRORS_R_XMLSEC_FAILED,
- "node=%s",
- xmlSecErrorsSafeString(xmlSecNodeGetName(node)));
- return(-1);
- }
- encCtx->resultReplaced = 1;
- } else {
- /* we should've catached this error before */
- xmlSecError(XMLSEC_ERRORS_HERE,
- NULL,
- NULL,
- XMLSEC_ERRORS_R_INVALID_TYPE,
- "type=%s",
- xmlSecErrorsSafeString(encCtx->type));
- return(-1);
+ NULL,
+ NULL,
+ XMLSEC_ERRORS_R_INVALID_TYPE,
+ "type=%s",
+ xmlSecErrorsSafeString(encCtx->type));
+ return(-1);
}
return(0);
}
@@ -589,31 +630,62 @@
/* replace original node if requested */
if((encCtx->type != NULL) && xmlStrEqual(encCtx->type, xmlSecTypeEncElement)) {
- ret = xmlSecReplaceNodeBuffer(node, xmlSecBufferGetData(buffer), xmlSecBufferGetSize(buffer));
- if(ret < 0) {
- xmlSecError(XMLSEC_ERRORS_HERE,
- NULL,
- "xmlSecReplaceNodeBuffer",
- XMLSEC_ERRORS_R_XMLSEC_FAILED,
- "node=%s",
- xmlSecErrorsSafeString(xmlSecNodeGetName(node)));
- return(-1);
- }
- encCtx->resultReplaced = 1;
+ /* check if we need to return the replaced node */
+ if((encCtx->flags & XMLSEC_ENC_RETURN_REPLACED_NODE) != 0) {
+ ret = xmlSecReplaceNodeBufferAndReturn(node, xmlSecBufferGetData(buffer), xmlSecBufferGetSize(buffer), &(encCtx->replacedNodeList));
+ if(ret < 0) {
+ xmlSecError(XMLSEC_ERRORS_HERE,
+ NULL,
+ "xmlSecReplaceNodeBufferAndReturn",
+ XMLSEC_ERRORS_R_XMLSEC_FAILED,
+ "node=%s",
+ xmlSecErrorsSafeString(xmlSecNodeGetName(node)));
+ return(-1);
+ }
+ } else {
+ ret = xmlSecReplaceNodeBuffer(node, xmlSecBufferGetData(buffer), xmlSecBufferGetSize(buffer));
+ if(ret < 0) {
+ xmlSecError(XMLSEC_ERRORS_HERE,
+ NULL,
+ "xmlSecReplaceNodeBuffer",
+ XMLSEC_ERRORS_R_XMLSEC_FAILED,
+ "node=%s",
+ xmlSecErrorsSafeString(xmlSecNodeGetName(node)));
+ return(-1);
+ }
+ }
+
+ encCtx->resultReplaced = 1;
} else if((encCtx->type != NULL) && xmlStrEqual(encCtx->type, xmlSecTypeEncContent)) {
- /* replace the node with the buffer */
- ret = xmlSecReplaceNodeBuffer(node, xmlSecBufferGetData(buffer), xmlSecBufferGetSize(buffer));
- if(ret < 0) {
- xmlSecError(XMLSEC_ERRORS_HERE,
- NULL,
- "xmlSecReplaceNodeBuffer",
- XMLSEC_ERRORS_R_XMLSEC_FAILED,
- "node=%s",
- xmlSecErrorsSafeString(xmlSecNodeGetName(node)));
- return(-1);
- }
- encCtx->resultReplaced = 1;
+ /* replace the node with the buffer */
+
+ /* check if we need to return the replaced node */
+ if((encCtx->flags & XMLSEC_ENC_RETURN_REPLACED_NODE) != 0) {
+ ret = xmlSecReplaceNodeBufferAndReturn(node, xmlSecBufferGetData(buffer), xmlSecBufferGetSize(buffer), &(encCtx->replacedNodeList));
+ if(ret < 0) {
+ xmlSecError(XMLSEC_ERRORS_HERE,
+ NULL,
+ "xmlSecReplaceNodeBufferAndReturn",
+ XMLSEC_ERRORS_R_XMLSEC_FAILED,
+ "node=%s",
+ xmlSecErrorsSafeString(xmlSecNodeGetName(node)));
+ return(-1);
+ }
+ } else {
+ ret = xmlSecReplaceNodeBuffer(node, xmlSecBufferGetData(buffer), xmlSecBufferGetSize(buffer));
+ if(ret < 0) {
+ xmlSecError(XMLSEC_ERRORS_HERE,
+ NULL,
+ "xmlSecReplaceNodeBuffer",
+ XMLSEC_ERRORS_R_XMLSEC_FAILED,
+ "node=%s",
+ xmlSecErrorsSafeString(xmlSecNodeGetName(node)));
+ return(-1);
+ }
+ }
+ encCtx->resultReplaced = 1;
}
+
return(0);
}
Index: include/xmlsec/xmltree.h
===================================================================
--- include/xmlsec/xmltree.h (revision 988)
+++ include/xmlsec/xmltree.h (working copy)
@@ -56,11 +56,24 @@
XMLSEC_EXPORT int xmlSecReplaceNode (xmlNodePtr node,
xmlNodePtr newNode);
+XMLSEC_EXPORT int xmlSecReplaceNodeAndReturn
+ (xmlNodePtr node,
+ xmlNodePtr newNode,
+ xmlNodePtr* replaced);
XMLSEC_EXPORT int xmlSecReplaceContent (xmlNodePtr node,
xmlNodePtr newNode);
+XMLSEC_EXPORT int xmlSecReplaceContentAndReturn
+ (xmlNodePtr node,
+ xmlNodePtr newNode,
+ xmlNodePtr* replaced);
XMLSEC_EXPORT int xmlSecReplaceNodeBuffer (xmlNodePtr node,
const xmlSecByte *buffer,
xmlSecSize size);
+XMLSEC_EXPORT int xmlSecReplaceNodeBufferAndReturn
+ (xmlNodePtr node,
+ const xmlSecByte *buffer,
+ xmlSecSize size,
+ xmlNodePtr* replaced);
XMLSEC_EXPORT void xmlSecAddIDs (xmlDocPtr doc,
xmlNodePtr cur,
Index: include/xmlsec/xmlenc.h
===================================================================
--- include/xmlsec/xmlenc.h (revision 988)
+++ include/xmlsec/xmlenc.h (working copy)
@@ -41,6 +41,14 @@
xmlEncCtxModeEncryptedKey
} xmlEncCtxMode;
+
+/**
+ * XMLSEC_ENC_RETURN_REPLACED_NODE:
+ *
+ * If this flag is set, then the replaced node will be returned in the replacedNodeList
+ */
+#define XMLSEC_ENC_RETURN_REPLACED_NODE 0x00000001
+
/**
* xmlSecEncCtx:
* @userData: the pointer to user data (xmlsec and xmlsec-crypto libraries
@@ -61,6 +69,7 @@
* @resultReplaced: the flag: if set then resulted <enc:EncryptedData/>
* or <enc:EncryptedKey/> node is added to the document.
* @encMethod: the pointer to encryption transform.
+ * @replacedNodeList: the first node of the list of replaced nodes depending on the nodeReplacementMode
* @id: the ID attribute of <enc:EncryptedData/>
* or <enc:EncryptedKey/> node.
* @type: the Type attribute of <enc:EncryptedData/>
@@ -76,7 +85,6 @@
* @encMethodNode: the pointer to <enc:EncryptionMethod/> node.
* @keyInfoNode: the pointer to <enc:KeyInfo/> node.
* @cipherValueNode: the pointer to <enc:CipherValue/> node.
- * @reserved0: reserved for the future.
* @reserved1: reserved for the future.
*
* XML Encrypiton context.
@@ -99,7 +107,7 @@
int resultBase64Encoded;
int resultReplaced;
xmlSecTransformPtr encMethod;
-
+
/* attributes from EncryptedData or EncryptedKey */
xmlChar* id;
xmlChar* type;
@@ -113,10 +121,9 @@
xmlNodePtr encMethodNode;
xmlNodePtr keyInfoNode;
xmlNodePtr cipherValueNode;
-
- /* reserved for future */
- void* reserved0;
- void* reserved1;
+
+ xmlNodePtr replacedNodeList; /* the pointer to the replaced node */
+ void* reserved1; /* reserved for future */
};
XMLSEC_EXPORT xmlSecEncCtxPtr xmlSecEncCtxCreate (xmlSecKeysMngrPtr keysMngr);
More information about the xmlsec
mailing list