Bug 13305 - off-by-one error in _dbus_message_iter_get_args_valist()
Summary: off-by-one error in _dbus_message_iter_get_args_valist()
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: unspecified
Hardware: Other All
: high normal
Assignee: John (J5) Palmieri
QA Contact: John (J5) Palmieri
Keywords: patch
Depends on:
Reported: 2007-11-19 15:16 UTC by Tomas Carnecky
Modified: 2013-10-08 12:10 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:

client listening for events (1.21 KB, text/plain)
2007-11-19 15:17 UTC, Tomas Carnecky
server broadcasting events (1.10 KB, text/plain)
2007-11-19 15:18 UTC, Tomas Carnecky
Patch to fix the off by one error message (3.41 KB, patch)
2007-11-20 13:03 UTC, John (J5) Palmieri
Details | Splinter Review
[PATCH] fix off by one error message (#13305) (2.92 KB, patch)
2013-09-29 10:18 UTC, Chengwei Yang
Details | Splinter Review

Description Tomas Carnecky 2007-11-19 15:16:39 UTC

please see the two attached programs (server + client)

The client prints 'Message parsing error: Message has only 1 arguments, but more were expected' while the message had two arguments attached.
Comment 1 Tomas Carnecky 2007-11-19 15:17:28 UTC
Created attachment 12630 [details]
client listening for events
Comment 2 Tomas Carnecky 2007-11-19 15:18:38 UTC
Created attachment 12631 [details]
server broadcasting events
Comment 3 John (J5) Palmieri 2007-11-20 07:47:43 UTC
ok, after following your code I do see the off by one error.  Thanks for the test case.  I need to check if the semantics are different with arrays present but otherwise it should be an easy fix.
Comment 4 Havoc Pennington 2007-11-20 10:58:28 UTC
Is the bug here just that the error message has the wrong number in it?
Comment 5 John (J5) Palmieri 2007-11-20 11:04:09 UTC
Yes, it is simply the error message though there is code that resets i and reuses it as a counter when an array is seen so it may be completely broken in that case.  I'm investigating.
Comment 6 Tomas Carnecky 2007-11-20 11:11:28 UTC
If it's too complicated to properly fix, then at least change the error message to _not_ print the number, but instead just say 'message has less arguments than expected'. But if you print a number that is wrong it will confuse the users (well, it did confuse me...).
Comment 7 John (J5) Palmieri 2007-11-20 12:31:55 UTC
yep that array counter is wrong too.  Sending in a 4 element array instead of
the string gives me the error message:

Message parsing error: Message has only 4 arguments, but more were expected

Patch coming
Comment 8 John (J5) Palmieri 2007-11-20 13:03:42 UTC
Created attachment 12653 [details] [review]
Patch to fix the off by one error message

This should do it
Comment 9 Havoc Pennington 2007-11-20 13:09:58 UTC
The patch contains these two lines:

              j = 0;
              while (i < n_elements)

That looks unlikely to be correct? (i/j mismatch)

Can't the array-iteration variable be local to the block that does the array iteration, reducing the potential for bugs?

Even nicer, maybe break the array part out into a separate function (passing it a "va_list*" probably)

Comment 10 John (J5) Palmieri 2008-01-14 15:14:03 UTC
woops, went off to other things and forgot about this one.  I'll have to rewrite the patch so it isn't going to make the next release.  I'll make sure to do something for 1.2.0 release though. Moving this to high priority just to keep it at the top of my queue.
Comment 11 Simon McVittie 2011-01-19 07:16:46 UTC
(In reply to comment #10)
> woops, went off to other things and forgot about this one.

Comment 12 Chengwei Yang 2013-09-29 09:54:44 UTC
Comment on attachment 12653 [details] [review]
Patch to fix the off by one error message

Review of attachment 12653 [details] [review]:

I'll upload a new version based on J5's patch soon.

::: dbus/dbus-message.c
@@ +754,1 @@
>                while (i < n_elements)

fail, should be j here.

@@ +758,3 @@
>                                                  &s);
>                    str_array[i] = _dbus_strdup (s);

fail, should be j here.
Comment 13 Chengwei Yang 2013-09-29 10:18:26 UTC
Created attachment 86791 [details] [review]
[PATCH] fix off by one error message (#13305)
Comment 14 Simon McVittie 2013-10-08 12:10:17 UTC
This is basically cosmetic (i and j are only used in error messages) so I'm not going to put this in the 1.6 stable branch. Fixed in git for 1.7.6.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.