Project

Profile

Help

HostedRedmine.com has moved to the Planio platform. All logins and passwords remained the same. All users will be able to login and use Redmine just as before. Read more...

Bug #863750

C undefined behaviour in specvec _copy with zero-length source

Added by Jacob Nevins over 1 year ago. Updated over 1 year ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
General
Sprint/Milestone:
Start date:
Due date:
% Done:

0%

Estimated time:

Description

It turns out that memcpy(NULL, _, 0) is undefined behaviour (sigh), and that this can actually matter. This article has a good explanation.

Legal usage of specvec -- copying a zero-length vector -- can invoke this.

I noticed this when starting a server compiled with UBSan (gcc -fsanitize=undefined):

...
2: Loading rulesets.
/home/jtn/src/freeciv/git26/utility/specvec.h:117:3: runtime error: null pointer passed as argument 1, which is declared to never be null
    #0 0x556be4152a18 in requirement_vector_copy /home/jtn/src/freeciv/git26/utility/specvec.h:117
    #1 0x556be4188303 in load_ruleset_cities /home/jtn/src/freeciv/git26/server/ruleset.c:4820
    #2 0x556be41a5f6d in load_rulesetdir /home/jtn/src/freeciv/git26/server/ruleset.c:7067
    #3 0x556be41a5a44 in load_rulesets /home/jtn/src/freeciv/git26/server/ruleset.c:6938
    #4 0x556be3e95c37 in srv_prepare /home/jtn/src/freeciv/git26/server/srv_main.c:2939
    #5 0x556be3e99fd5 in srv_main /home/jtn/src/freeciv/git26/server/srv_main.c:3342
    #6 0x556be3e720d3 in main /home/jtn/src/freeciv/git26/server/civserver.c:476
    #7 0x7f303298a09a in __libc_start_main ../csu/libc-start.c:308
    #8 0x556be3e6f1e9 in _start (/home/jtn/src/freeciv/git26/build-asan/server/freeciv-server+0xe331e9)

2: AI*1 has been added as Easy level AI-controlled player (classic).
...

(This is setting up the specialist reqs vectors, which are all empty in the 2.6 default ruleset.)

I've no evidence that this actually caused a user-visible bug with common toolchains.

I haven't gone out of my way to audit for other instances.

m-30-26-specvec-ub.patch (928 Bytes) m-30-26-specvec-ub.patch master, S3_0, S2_6 Jacob Nevins, 2020-03-07 04:21 PM

History

#1 Updated by Jacob Nevins over 1 year ago

#2 Updated by Chippo Elder over 1 year ago

I used S3_0, clang and UBSan and replicated the warning:

2: Loading rulesets.
../utility/specvec.h:117:10: runtime error: null pointer passed as argument 1, which is declared to never be null
/usr/include/string.h:43:28: note: nonnull attribute specified here
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../utility/specvec.h:117:10 in 
../utility/specvec.h:117:17: runtime error: null pointer passed as argument 2, which is declared to never be null
/usr/include/string.h:43:28: note: nonnull attribute specified here
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../utility/specvec.h:117:17 in 

The patch didn't apply, though it looks fine to me:

chippo@chippo-Aspire-V3-731:~/Downloads/git_clones/freeciv/freeciv-30$ git apply ../m-30-26-specvec-ub.patch
error: specvec.h: No such file or directory

So, I applied it with vi

chippo@chippo-Aspire-V3-731:~/Downloads/git_clones/freeciv/freeciv-30$ git diff
diff --git a/utility/specvec.h b/utility/specvec.h
index 22627d07eb..7102873396 100644
--- a/utility/specvec.h
+++ b/utility/specvec.h
@@ -114,7 +114,9 @@ static inline void SPECVEC_FOO(_vector_copy) (SPECVEC_VECTOR *to,
                                              const SPECVEC_VECTOR *from)
 {
   SPECVEC_FOO(_vector_reserve) (to, from->size);
-  memcpy(to->p, from->p, from->size * sizeof(*to->p));
+  if (from->size > 0) {
+    memcpy(to->p, from->p, from->size * sizeof(*to->p));
+  }
 }

 static inline void SPECVEC_FOO(_vector_free) (SPECVEC_VECTOR *tthis)

and can confirm that it takes the warnings away for llvm UBSan and S3_0.

#3 Updated by Jacob Nevins over 1 year ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF