<p>Vadim Yanitskiy has uploaded this change for <strong>review</strong>.</p><p><a href="https://gerrit.osmocom.org/13470">View Change</a></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">libmsc/db.c: fix incorrect SMS UD length truncation<br><br>During the compilation, clang-4.0 with -Wall writes the following:<br><br> db.c:278:25: warning: comparison of constant 256 with expression of<br> type 'uint8_t' (aka 'unsigned char') is always false<br> [-Wtautological-constant-out-of-range-compare]<br> if (sms->user_data_len > sizeof(sms->user_data))<br> ~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~<br> db.c:424:25: warning: comparison of constant 256 with expression of<br> type 'uint8_t' (aka 'unsigned char') is always false<br> [-Wtautological-constant-out-of-range-compare]<br> if (sms->user_data_len > sizeof(sms->user_data))<br> ~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~<br> db.c:780:25: warning: comparison of constant 256 with expression of<br> type 'uint8_t' (aka 'unsigned char') is always false<br> [-Wtautological-constant-out-of-range-compare]<br> if (sms->user_data_len > sizeof(sms->user_data))<br> ~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~<br><br>Given that 'sizeof(sms->user_data)' is 256 (see SMS_TEXT_SIZE), and<br>'sms->user_data_len' is of type 'uint8_t', these warnings make sense.<br><br>The value of 'sms->user_data_len' is fetched from the database:<br><br> sms->user_data_len = dbi_result_get_field_length(result, "user_data");<br><br>and this is where the problem is. As per the libdbi's documentation<br>(see 3.5.3), dbi_result_get_field_length() returns the length in<br>bytes of the value stored in the specified field:<br><br> unsigned int dbi_result_get_field_length(dbi_result Result,<br> const char *fieldname)<br><br>so 'unsigned int' is assigned to 'uint8_t', what would lead to<br>integer overflow if the value is grather than 0xff. As a result,<br>the truncation of 'user_data' would be done incorrectly, e.g.<br>if the length of 'user_data' is 282, only 26 bytes would be<br>fetched instead of 256.<br><br>Let's avoid such direct assignment, and use a separate variable.<br><br>Change-Id: Ibbd588545e1a4817504c806a3d02cf59d5938ee2<br>Related: OS#3684<br>---<br>M src/libmsc/db.c<br>1 file changed, 15 insertions(+), 9 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/70/13470/1</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/src/libmsc/db.c b/src/libmsc/db.c</span><br><span>index b5e7ad8..55a87bb 100644</span><br><span>--- a/src/libmsc/db.c</span><br><span>+++ b/src/libmsc/db.c</span><br><span>@@ -236,6 +236,7 @@</span><br><span> long long unsigned int sender_id;</span><br><span> const char *text, *daddr;</span><br><span> const unsigned char *user_data;</span><br><span style="color: hsl(120, 100%, 40%);">+ unsigned int user_data_len;</span><br><span> char buf[32];</span><br><span> char *quoted;</span><br><span> dbi_result result2;</span><br><span>@@ -273,10 +274,11 @@</span><br><span> if (daddr)</span><br><span> OSMO_STRLCPY_ARRAY(sms->dst.addr, daddr);</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- sms->user_data_len = dbi_result_get_field_length(result, "user_data");</span><br><span style="color: hsl(120, 100%, 40%);">+ user_data_len = dbi_result_get_field_length(result, "user_data");</span><br><span> user_data = dbi_result_get_binary(result, "user_data");</span><br><span style="color: hsl(0, 100%, 40%);">- if (sms->user_data_len > sizeof(sms->user_data))</span><br><span style="color: hsl(0, 100%, 40%);">- sms->user_data_len = (uint8_t) sizeof(sms->user_data);</span><br><span style="color: hsl(120, 100%, 40%);">+ if (user_data_len > sizeof(sms->user_data))</span><br><span style="color: hsl(120, 100%, 40%);">+ user_data_len = (uint8_t) sizeof(sms->user_data);</span><br><span style="color: hsl(120, 100%, 40%);">+ sms->user_data_len = user_data_len;</span><br><span> memcpy(sms->user_data, user_data, sms->user_data_len);</span><br><span> </span><br><span> text = dbi_result_get_string(result, "text");</span><br><span>@@ -395,6 +397,7 @@</span><br><span> {</span><br><span> struct gsm_sms *sms = sms_alloc();</span><br><span> const unsigned char *user_data;</span><br><span style="color: hsl(120, 100%, 40%);">+ unsigned int user_data_len;</span><br><span> const char *text, *addr;</span><br><span> </span><br><span> if (!sms)</span><br><span>@@ -419,10 +422,11 @@</span><br><span> sms->dst.ton = dbi_result_get_ulonglong(result, "dest_ton");</span><br><span> sms->dst.npi = dbi_result_get_ulonglong(result, "dest_npi");</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- sms->user_data_len = dbi_result_get_field_length(result, "user_data");</span><br><span style="color: hsl(120, 100%, 40%);">+ user_data_len = dbi_result_get_field_length(result, "user_data");</span><br><span> user_data = dbi_result_get_binary(result, "user_data");</span><br><span style="color: hsl(0, 100%, 40%);">- if (sms->user_data_len > sizeof(sms->user_data))</span><br><span style="color: hsl(0, 100%, 40%);">- sms->user_data_len = (uint8_t) sizeof(sms->user_data);</span><br><span style="color: hsl(120, 100%, 40%);">+ if (user_data_len > sizeof(sms->user_data))</span><br><span style="color: hsl(120, 100%, 40%);">+ user_data_len = (uint8_t) sizeof(sms->user_data);</span><br><span style="color: hsl(120, 100%, 40%);">+ sms->user_data_len = user_data_len;</span><br><span> memcpy(sms->user_data, user_data, sms->user_data_len);</span><br><span> </span><br><span> text = dbi_result_get_string(result, "text");</span><br><span>@@ -744,6 +748,7 @@</span><br><span> struct gsm_sms *sms = sms_alloc();</span><br><span> const char *text, *daddr, *saddr;</span><br><span> const unsigned char *user_data;</span><br><span style="color: hsl(120, 100%, 40%);">+ unsigned int user_data_len;</span><br><span> time_t validity_timestamp;</span><br><span> </span><br><span> if (!sms)</span><br><span>@@ -777,10 +782,11 @@</span><br><span> if (saddr)</span><br><span> OSMO_STRLCPY_ARRAY(sms->src.addr, saddr);</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">- sms->user_data_len = dbi_result_get_field_length(result, "user_data");</span><br><span style="color: hsl(120, 100%, 40%);">+ user_data_len = dbi_result_get_field_length(result, "user_data");</span><br><span> user_data = dbi_result_get_binary(result, "user_data");</span><br><span style="color: hsl(0, 100%, 40%);">- if (sms->user_data_len > sizeof(sms->user_data))</span><br><span style="color: hsl(0, 100%, 40%);">- sms->user_data_len = (uint8_t) sizeof(sms->user_data);</span><br><span style="color: hsl(120, 100%, 40%);">+ if (user_data_len > sizeof(sms->user_data))</span><br><span style="color: hsl(120, 100%, 40%);">+ user_data_len = (uint8_t) sizeof(sms->user_data);</span><br><span style="color: hsl(120, 100%, 40%);">+ sms->user_data_len = user_data_len;</span><br><span> memcpy(sms->user_data, user_data, sms->user_data_len);</span><br><span> </span><br><span> text = dbi_result_get_string(result, "text");</span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.osmocom.org/13470">change 13470</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/13470"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: osmo-msc </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: newchange </div>
<div style="display:none"> Gerrit-Change-Id: Ibbd588545e1a4817504c806a3d02cf59d5938ee2 </div>
<div style="display:none"> Gerrit-Change-Number: 13470 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Vadim Yanitskiy <axilirator@gmail.com> </div>