Ticket #801: opensync-pointer-checks

File opensync-pointer-checks, 8.3 KB (added by microe, 5 months ago)

Expanded patch. Same theme - fixes defective pointer checks (or adds them)

Line 
11. The OSyncMessage reply should be checked for null in
2   opensync_client.c. This patch does that.
3
42. opensync_client_proxy.c has error_free_message and
5   error_free_context reversed. So when error_free_context is called
6   the null message pointer will be dereferenced. Causing a segfault.
7
83.  opensync_queue.c might accidentally dereferenced a null errormsg.
9
104. In opensync_serializer.c auth is checked when *auth was the pointer
11   that was actually given the allocated authentication object. Change
12   the check to *auth.
13
145. At line 210 of opensync_merger.c new_node is checked for null. Then
15   at line 213 new_node is dereferenced to get the doc pointer for
16   the destination doc. Remove call (and commented out call).
17
186. In opensync_xml.c at line 219 the two pointers are compared. But at
19   line 223 they are checked for null. Since strcmp will dereference
20   both pointers to compare their strings this would cause a segfault.
21   Move the pointer test to before the strcmp.
22
237. osyncplugin.c checks a pointer but only reports it. So if it is
24   indeed null a later action might dereference it. The fix is to
25   error out after reporting.
26
27Signed-off-by: Erik Hovland <erik@hovland.org>
28
29---
30
31 opensync/client/opensync_client.c       |    9 +++++++++
32 opensync/client/opensync_client_proxy.c |   24 ++++++++++++------------
33 opensync/ipc/opensync_queue.c           |    3 ++-
34 opensync/ipc/opensync_serializer.c      |    2 +-
35 opensync/merger/opensync_merger.c       |    2 --
36 opensync/opensync_xml.c                 |    8 ++++----
37 tools/osyncplugin.c                     |    4 +++-
38 7 files changed, 31 insertions(+), 21 deletions(-)
39
40diff --git a/opensync/client/opensync_client.c b/opensync/client/opensync_client.c
41index c22d982..bda43fd 100644
42--- a/opensync/client/opensync_client.c
43+++ b/opensync/client/opensync_client.c
44@@ -113,6 +113,9 @@ static void _osync_client_connect_callback(void *data, OSyncError *error)
45        OSyncMessage *reply = NULL;
46        if (!osync_error_is_set(&error)) {
47                reply = osync_message_new_reply(message, &locerror);
48+               if (!reply)
49+                       goto error;
50+
51                //Send connect specific reply data
52                osync_message_write_int(reply, slowsync);
53 
54@@ -300,6 +303,9 @@ static void _osync_client_read_callback(void *data, OSyncError *error)
55        OSyncMessage *reply = NULL;
56        if (!osync_error_is_set(&error)) {
57                reply = osync_message_new_reply(message, &locerror);
58+               if (!reply)
59+                       goto error;
60+
61                //Send get_changes specific reply data
62                osync_message_write_string(reply, osync_change_get_uid(baton->change));
63        } else {
64@@ -344,6 +350,9 @@ static void _osync_client_commit_change_callback(void *data, OSyncError *error)
65        OSyncMessage *reply = NULL;
66        if (!osync_error_is_set(&error)) {
67                reply = osync_message_new_reply(message, &locerror);
68+               if (!reply)
69+                       goto error;
70+
71                //Send get_changes specific reply data
72                osync_message_write_string(reply, osync_change_get_uid(baton->change));
73        } else {
74diff --git a/opensync/client/opensync_client_proxy.c b/opensync/client/opensync_client_proxy.c
75index 3cd2cfb..f094632 100644
76--- a/opensync/client/opensync_client_proxy.c
77+++ b/opensync/client/opensync_client_proxy.c
78@@ -1274,10 +1274,10 @@ osync_bool osync_client_proxy_disconnect(OSyncClientProxy *proxy, disconnect_cb
79        osync_trace(TRACE_EXIT, "%s", __func__);
80        return TRUE;
81 
82-error_free_context:
83-       g_free(ctx);
84 error_free_message:
85        osync_message_unref(message);
86+error_free_context:
87+       g_free(ctx);
88 error:
89        osync_trace(TRACE_EXIT_ERROR, "%s: %s", __func__, osync_error_print(error));
90        return FALSE;
91@@ -1318,10 +1318,10 @@ osync_bool osync_client_proxy_read(OSyncClientProxy *proxy, read_cb callback, vo
92        osync_trace(TRACE_EXIT, "%s", __func__);
93        return TRUE;
94 
95-error_free_context:
96-       g_free(ctx);
97 error_free_message:
98        osync_message_unref(message);
99+error_free_context:
100+       g_free(ctx);
101 error:
102        osync_trace(TRACE_EXIT_ERROR, "%s: %s", __func__, osync_error_print(error));
103        return FALSE;
104@@ -1362,10 +1362,10 @@ osync_bool osync_client_proxy_get_changes(OSyncClientProxy *proxy, get_changes_c
105        osync_trace(TRACE_EXIT, "%s", __func__);
106        return TRUE;
107 
108-error_free_context:
109-       g_free(ctx);
110 error_free_message:
111        osync_message_unref(message);
112+error_free_context:
113+       g_free(ctx);
114 error:
115        osync_trace(TRACE_EXIT_ERROR, "%s: %s", __func__, osync_error_print(error));
116        return FALSE;
117@@ -1408,10 +1408,10 @@ osync_bool osync_client_proxy_commit_change(OSyncClientProxy *proxy, commit_chan
118        osync_trace(TRACE_EXIT, "%s", __func__);
119        return TRUE;
120 
121-error_free_context:
122-       g_free(ctx);
123 error_free_message:
124        osync_message_unref(message);
125+error_free_context:
126+       g_free(ctx);
127 error:
128        osync_trace(TRACE_EXIT_ERROR, "%s: %s", __func__, osync_error_print(error));
129        return FALSE;
130@@ -1452,10 +1452,10 @@ osync_bool osync_client_proxy_committed_all(OSyncClientProxy *proxy, committed_a
131        osync_trace(TRACE_EXIT, "%s", __func__);
132        return TRUE;
133 
134-error_free_context:
135-       g_free(ctx);
136 error_free_message:
137        osync_message_unref(message);
138+error_free_context:
139+       g_free(ctx);
140 error:
141        osync_trace(TRACE_EXIT_ERROR, "%s: %s", __func__, osync_error_print(error));
142        return FALSE;
143@@ -1496,10 +1496,10 @@ osync_bool osync_client_proxy_sync_done(OSyncClientProxy *proxy, sync_done_cb ca
144        osync_trace(TRACE_EXIT, "%s", __func__);
145        return TRUE;
146 
147-error_free_context:
148-       g_free(ctx);
149 error_free_message:
150        osync_message_unref(message);
151+error_free_context:
152+       g_free(ctx);
153 error:
154        osync_trace(TRACE_EXIT_ERROR, "%s: %s", __func__, osync_error_print(error));
155        return FALSE;
156diff --git a/opensync/ipc/opensync_queue.c b/opensync/ipc/opensync_queue.c
157index bda943e..db0eb66 100644
158--- a/opensync/ipc/opensync_queue.c
159+++ b/opensync/ipc/opensync_queue.c
160@@ -136,7 +136,8 @@ gboolean _timeout_dispatch(GSource *source, GSourceFunc callback, gpointer user_
161                        g_mutex_unlock(queue->pendingLock);
162 
163                        pending->callback(errormsg, pending->user_data);
164-                       osync_message_unref(errormsg);
165+                       if (errormsg != NULL)
166+                               osync_message_unref(errormsg);
167 
168                        // TODO: Refcounting for OSyncPendingMessage
169                        g_free(pending->timeout_info);
170diff --git a/opensync/ipc/opensync_serializer.c b/opensync/ipc/opensync_serializer.c
171index 5f0563f..74fce15 100644
172--- a/opensync/ipc/opensync_serializer.c
173+++ b/opensync/ipc/opensync_serializer.c
174@@ -1164,7 +1164,7 @@ osync_bool osync_demarshal_pluginauthentication(OSyncMessage *message, OSyncPlug
175        char *reference = NULL;
176 
177        *auth = osync_plugin_authentication_new(error);
178-       if (!auth)
179+       if (!*auth)
180                goto error;
181 
182        osync_message_read_uint(message, &available_fields);
183diff --git a/opensync/merger/opensync_merger.c b/opensync/merger/opensync_merger.c
184index a86b7f5..09513fa 100644
185--- a/opensync/merger/opensync_merger.c
186+++ b/opensync/merger/opensync_merger.c
187@@ -210,8 +210,6 @@ void osync_merger_merge(OSyncMerger *merger, OSyncXMLFormat *xmlformat, OSyncXML
188                                                if(new_node == NULL) {
189                                                        for(tmp=list; tmp != NULL; tmp = g_slist_next(tmp)) {
190                                                                xmlUnlinkNode(tmp->data);
191-                                                               xmlDOMWrapAdoptNode(NULL, ((xmlNodePtr)tmp->data)->doc, tmp->data, new_node->doc, new_par_node, 0);
192-                                                               //xmlAddChild(new_par_node, tmp->data);
193                                                        }
194                                                }else{
195                                                        for(tmp=list; tmp != NULL; tmp = g_slist_next(tmp)) {
196diff --git a/opensync/opensync_xml.c b/opensync/opensync_xml.c
197index 654a199..5614dbe 100644
198--- a/opensync/opensync_xml.c
199+++ b/opensync/opensync_xml.c
200@@ -216,14 +216,14 @@ static osync_bool osync_xml_compare_node(xmlNode *leftnode, xmlNode *rightnode)
201                                g_free(rightcontent);
202                                goto next;
203                        }
204-                       if (!strcmp(leftcontent, rightcontent)) {
205-                               g_free(rightcontent);
206-                               goto next;
207-                       }
208                        if (!leftcontent || !rightcontent) {
209                                osync_trace(TRACE_EXIT, "%s: One is empty", __func__);
210                                return FALSE;
211                        }
212+                       if (!strcmp(leftcontent, rightcontent)) {
213+                               g_free(rightcontent);
214+                               goto next;
215+                       }
216                        g_free(rightcontent);
217                } while ((rightnode = rightnode->next));
218                osync_trace(TRACE_EXIT, "%s: Could not match one", __func__);
219diff --git a/tools/osyncplugin.c b/tools/osyncplugin.c
220index 48c2b06..d666540 100644
221--- a/tools/osyncplugin.c
222+++ b/tools/osyncplugin.c
223@@ -1057,8 +1057,10 @@ static osync_bool run_command(Command *cmd, void **plugin_data, OSyncError **err
224 
225        assert(cmd);
226 
227-       if (cmd->cmd != CMD_INITIALIZE && *plugin_data == NULL)
228+       if (cmd->cmd != CMD_INITIALIZE && *plugin_data == NULL) {
229                fprintf(stderr, "WARNING: Got Plugin initialized? plugin_data is NULL.\n");
230+               goto error;
231+       }
232 
233 
234        switch (cmd->cmd) {